MediaRecorder stopRecorder() returns empty Blob after first use
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Jun 2020 15:58:41 +0000 (15:58 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Jun 2020 15:58:41 +0000 (15:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=212274
<rdar://problem/63601298>

Reviewed by Eric Carlson.

Source/WebCore:

Refactor code to create/destroy MediaRecorderPrivate on MediaRecorder start/stop.
This allows reusing a MediaRecorder after a stop and restarting with a clean state.

We introduce MediaRecorderPrivate::startRecording to do the initialization,
which allows to fix a potential ref cycle as part of the error callback handling.

Make some improvements to the platform implementation, in particular add default initialization to all fields.
Align the code using AudioConverterRef to what is done in AudioSampleDataSource.
Also call VTCompressionSessionInvalidate when destroying the VideoSampleBufferCompressor.

Test: http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html

* Modules/mediarecorder/MediaRecorder.cpp:
(WebCore::MediaRecorder::create):
(WebCore::MediaRecorder::MediaRecorder):
(WebCore::MediaRecorder::startRecording):
(WebCore::MediaRecorder::stopRecording):
(WebCore::MediaRecorder::requestData):
* Modules/mediarecorder/MediaRecorder.h:
* platform/mediarecorder/MediaRecorderPrivate.h:
(WebCore::MediaRecorderPrivate::startRecording):
* platform/mediarecorder/cocoa/AudioSampleBufferCompressor.h:
* platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:
(WebCore::AudioSampleBufferCompressor::~AudioSampleBufferCompressor):
(WebCore::AudioSampleBufferCompressor::initAudioConverterForSourceFormatDescription):
(WebCore::AudioSampleBufferCompressor::attachPrimingTrimsIfNeeded):
(WebCore::AudioSampleBufferCompressor::gradualDecoderRefreshCount):
(WebCore::AudioSampleBufferCompressor::sampleBufferWithNumPackets):
(WebCore::AudioSampleBufferCompressor::processSampleBuffersUntilLowWaterTime):
* platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:
* platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:
(WebCore::MediaRecorderPrivateWriter::stopRecording):
* platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:
(WebCore::VideoSampleBufferCompressor::~VideoSampleBufferCompressor):

Source/WebKit:

Update implementation to do initialization as part of startRecording.

* GPUProcess/webrtc/RemoteMediaRecorderManager.cpp:
(WebKit::RemoteMediaRecorderManager::releaseRecorder):
Remove ASSERT as recorder creation in WebProcess is always ok while creation in GPUProcess may fail and m_recorders may not be populated.
* WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:
(WebKit::MediaRecorderPrivate::MediaRecorderPrivate):
(WebKit::MediaRecorderPrivate::startRecording):
* WebProcess/GPU/webrtc/MediaRecorderPrivate.h:

LayoutTests:

* http/wpt/mediarecorder/MediaRecorder-multiple-start-stop-expected.txt: Added.
* http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@263511 268f45cc-cd09-0410-ab3c-d52691b4dbfc

16 files changed:
LayoutTests/ChangeLog
LayoutTests/http/wpt/mediarecorder/MediaRecorder-multiple-start-stop-expected.txt [new file with mode: 0644]
LayoutTests/http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp
Source/WebCore/Modules/mediarecorder/MediaRecorder.h
Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h
Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.h
Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm
Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h
Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm
Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm
Source/WebKit/ChangeLog
Source/WebKit/GPUProcess/webrtc/RemoteMediaRecorderManager.cpp
Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp
Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.h

index ba5f279..e452a62 100644 (file)
@@ -1,3 +1,14 @@
+2020-06-25  Youenn Fablet  <youenn@apple.com>
+
+        MediaRecorder stopRecorder() returns empty Blob after first use
+        https://bugs.webkit.org/show_bug.cgi?id=212274
+        <rdar://problem/63601298>
+
+        Reviewed by Eric Carlson.
+
+        * http/wpt/mediarecorder/MediaRecorder-multiple-start-stop-expected.txt: Added.
+        * http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html: Added.
+
 2020-06-25  Karl Rackler  <rackler@apple.com>
 
         Remove expectation for fast/history/page-cache-indexed-closed-db.html as they are passing. 
diff --git a/LayoutTests/http/wpt/mediarecorder/MediaRecorder-multiple-start-stop-expected.txt b/LayoutTests/http/wpt/mediarecorder/MediaRecorder-multiple-start-stop-expected.txt
new file mode 100644 (file)
index 0000000..2f454c2
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS Make sure that MediaRecorder can be used after stopping it once 
+
diff --git a/LayoutTests/http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html b/LayoutTests/http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html
new file mode 100644 (file)
index 0000000..b916650
--- /dev/null
@@ -0,0 +1,49 @@
+<!doctype html>
+<html>
+<head>
+    <title>MediaRecorder Dataavailable</title>
+    <link rel="help" href="https://w3c.github.io/mediacapture-record/MediaRecorder.html#mediarecorder">
+    <script src="/resources/testharness.js"></script>
+    <script src="/resources/testharnessreport.js"></script>
+</head>
+<body>
+<canvas id="canvas" width="200" height="200">
+</canvas>
+<script>
+    promise_test(async t => {
+        const video = await navigator.mediaDevices.getUserMedia({ audio : true, video : true });
+        const recorder = new MediaRecorder(video);
+
+        assert_equals(recorder.stream, video);
+
+        let promise = new Promise(resolve => {
+            recorder.ondataavailable = t.step_func(blobEvent => {
+                if (blobEvent.data.size)
+                    resolve();
+            });
+        });
+
+        recorder.start();
+        let timer = setInterval(() => recorder.requestData(), 10);
+        await promise;
+        clearInterval(timer);
+
+        recorder.stop();
+
+        promise = new Promise(resolve => {
+            recorder.ondataavailable = t.step_func(blobEvent => {
+                if (blobEvent.data.size)
+                    resolve();
+            });
+        });
+
+        recorder.start();
+        timer = setInterval(() => recorder.requestData(), 10);
+        await promise;
+        clearInterval(timer);
+
+        recorder.stop();
+    }, 'Make sure that MediaRecorder can be used after stopping it once');
+</script>
+</body>
+</html>
index eb7d9a6..3f5d6c5 100644 (file)
@@ -1,3 +1,46 @@
+2020-06-25  Youenn Fablet  <youenn@apple.com>
+
+        MediaRecorder stopRecorder() returns empty Blob after first use
+        https://bugs.webkit.org/show_bug.cgi?id=212274
+        <rdar://problem/63601298>
+
+        Reviewed by Eric Carlson.
+
+        Refactor code to create/destroy MediaRecorderPrivate on MediaRecorder start/stop.
+        This allows reusing a MediaRecorder after a stop and restarting with a clean state.
+
+        We introduce MediaRecorderPrivate::startRecording to do the initialization,
+        which allows to fix a potential ref cycle as part of the error callback handling.
+
+        Make some improvements to the platform implementation, in particular add default initialization to all fields.
+        Align the code using AudioConverterRef to what is done in AudioSampleDataSource.
+        Also call VTCompressionSessionInvalidate when destroying the VideoSampleBufferCompressor.
+
+        Test: http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html
+
+        * Modules/mediarecorder/MediaRecorder.cpp:
+        (WebCore::MediaRecorder::create):
+        (WebCore::MediaRecorder::MediaRecorder):
+        (WebCore::MediaRecorder::startRecording):
+        (WebCore::MediaRecorder::stopRecording):
+        (WebCore::MediaRecorder::requestData):
+        * Modules/mediarecorder/MediaRecorder.h:
+        * platform/mediarecorder/MediaRecorderPrivate.h:
+        (WebCore::MediaRecorderPrivate::startRecording):
+        * platform/mediarecorder/cocoa/AudioSampleBufferCompressor.h:
+        * platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:
+        (WebCore::AudioSampleBufferCompressor::~AudioSampleBufferCompressor):
+        (WebCore::AudioSampleBufferCompressor::initAudioConverterForSourceFormatDescription):
+        (WebCore::AudioSampleBufferCompressor::attachPrimingTrimsIfNeeded):
+        (WebCore::AudioSampleBufferCompressor::gradualDecoderRefreshCount):
+        (WebCore::AudioSampleBufferCompressor::sampleBufferWithNumPackets):
+        (WebCore::AudioSampleBufferCompressor::processSampleBuffersUntilLowWaterTime):
+        * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:
+        * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:
+        (WebCore::MediaRecorderPrivateWriter::stopRecording):
+        * platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:
+        (WebCore::VideoSampleBufferCompressor::~VideoSampleBufferCompressor):
+
 2020-06-25  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][TFC] Use the flexing value as the base for the available horizontal space distribution
index 8581d5c..19b96ce 100644 (file)
@@ -51,11 +51,8 @@ ExceptionOr<Ref<MediaRecorder>> MediaRecorder::create(Document& document, Ref<Me
     auto privateInstance = MediaRecorder::createMediaRecorderPrivate(document, stream->privateStream());
     if (!privateInstance)
         return Exception { NotSupportedError, "The MediaRecorder is unsupported on this platform"_s };
-    auto recorder = adoptRef(*new MediaRecorder(document, WTFMove(stream), WTFMove(privateInstance), WTFMove(options)));
+    auto recorder = adoptRef(*new MediaRecorder(document, WTFMove(stream), WTFMove(options)));
     recorder->suspendIfNeeded();
-    recorder->m_private->setErrorCallback([recorder](auto&& exception) mutable {
-        recorder->dispatchError(WTFMove(*exception));
-    });
     return recorder;
 }
 
@@ -82,11 +79,10 @@ std::unique_ptr<MediaRecorderPrivate> MediaRecorder::createMediaRecorderPrivate(
 #endif
 }
 
-MediaRecorder::MediaRecorder(Document& document, Ref<MediaStream>&& stream, std::unique_ptr<MediaRecorderPrivate>&& privateImpl, Options&& option)
+MediaRecorder::MediaRecorder(Document& document, Ref<MediaStream>&& stream, Options&& option)
     : ActiveDOMObject(document)
     , m_options(WTFMove(option))
     , m_stream(WTFMove(stream))
-    , m_private(WTFMove(privateImpl))
 {
     m_tracks = WTF::map(m_stream->getTracks(), [] (auto&& track) -> Ref<MediaStreamTrackPrivate> {
         return track->privateTrack();
@@ -132,9 +128,26 @@ const char* MediaRecorder::activeDOMObjectName() const
 ExceptionOr<void> MediaRecorder::startRecording(Optional<int> timeslice)
 {
     UNUSED_PARAM(timeslice);
+    if (!m_isActive)
+        return Exception { InvalidStateError, "The MediaRecorder is not active"_s };
+
     if (state() != RecordingState::Inactive)
         return Exception { InvalidStateError, "The MediaRecorder's state must be inactive in order to start recording"_s };
-    
+
+    ASSERT(!m_private);
+    m_private = createMediaRecorderPrivate(*document(), m_stream->privateStream());
+
+    if (!m_private)
+        return Exception { NotSupportedError, "The MediaRecorder is unsupported on this platform"_s };
+
+    m_private->startRecording([this, pendingActivity = makePendingActivity(*this)](auto&& exception) mutable {
+        if (!m_isActive || !exception)
+            return;
+
+        stopRecordingInternal();
+        dispatchError(WTFMove(*exception));
+    });
+
     for (auto& track : m_tracks)
         track->addObserver(*this);
 
@@ -146,22 +159,17 @@ ExceptionOr<void> MediaRecorder::stopRecording()
 {
     if (state() == RecordingState::Inactive)
         return Exception { InvalidStateError, "The MediaRecorder's state cannot be inactive"_s };
-    
-    queueTaskKeepingObjectAlive(*this, TaskSource::Networking, [this] {
-        if (!m_isActive || state() == RecordingState::Inactive)
-            return;
 
-        stopRecordingInternal();
-        ASSERT(m_state == RecordingState::Inactive);
-        m_private->fetchData([this, pendingActivity = makePendingActivity(*this)](auto&& buffer, auto& mimeType) {
+    stopRecordingInternal();
+    auto& privateRecorder = *m_private;
+    privateRecorder.fetchData([this, pendingActivity = makePendingActivity(*this), privateRecorder = WTFMove(m_private)](auto&& buffer, auto& mimeType) {
+        queueTaskKeepingObjectAlive(*this, TaskSource::Networking, [this, buffer = WTFMove(buffer), mimeType]() mutable {
             if (!m_isActive)
                 return;
-    
             dispatchEvent(BlobEvent::create(eventNames().dataavailableEvent, Event::CanBubble::No, Event::IsCancelable::No, buffer ? Blob::create(buffer.releaseNonNull(), mimeType) : Blob::create()));
 
             if (!m_isActive)
                 return;
-
             dispatchEvent(Event::create(eventNames().stopEvent, Event::CanBubble::No, Event::IsCancelable::No));
         });
     });
@@ -174,10 +182,12 @@ ExceptionOr<void> MediaRecorder::requestData()
         return Exception { InvalidStateError, "The MediaRecorder's state cannot be inactive"_s };
 
     m_private->fetchData([this, pendingActivity = makePendingActivity(*this)](auto&& buffer, auto& mimeType) {
-        if (!m_isActive)
-            return;
+        queueTaskKeepingObjectAlive(*this, TaskSource::Networking, [this, buffer = WTFMove(buffer), mimeType]() mutable {
+            if (!m_isActive)
+                return;
 
-        dispatchEvent(BlobEvent::create(eventNames().dataavailableEvent, Event::CanBubble::No, Event::IsCancelable::No, buffer ? Blob::create(buffer.releaseNonNull(), mimeType) : Blob::create()));
+            dispatchEvent(BlobEvent::create(eventNames().dataavailableEvent, Event::CanBubble::No, Event::IsCancelable::No, buffer ? Blob::create(buffer.releaseNonNull(), mimeType) : Blob::create()));
+        });
     });
     return { };
 }
index 2fd9ea5..8f6634c 100644 (file)
@@ -75,7 +75,7 @@ public:
     MediaStream& stream() { return m_stream.get(); }
 
 private:
-    MediaRecorder(Document&, Ref<MediaStream>&&, std::unique_ptr<MediaRecorderPrivate>&&, Options&& = { });
+    MediaRecorder(Document&, Ref<MediaStream>&&, Options&& = { });
 
     static std::unique_ptr<MediaRecorderPrivate> createMediaRecorderPrivate(Document&, MediaStreamPrivate&);
     
index 9ea81c8..04ba6b0 100644 (file)
@@ -60,16 +60,13 @@ public:
     virtual void fetchData(FetchDataCallback&&) = 0;
     virtual void stopRecording() = 0;
 
-    using ErrorCallback = Function<void(Optional<Exception>&&)>;
-    void setErrorCallback(ErrorCallback&& errorCallback) { m_errorCallback = WTFMove(errorCallback); }
+    using ErrorCallback = CompletionHandler<void(Optional<Exception>&&)>;
+    virtual void startRecording(ErrorCallback&& callback) { callback({ }); }
 
 protected:
     void setAudioSource(RefPtr<RealtimeMediaSource>&&);
     void setVideoSource(RefPtr<RealtimeMediaSource>&&);
 
-protected:
-    ErrorCallback m_errorCallback;
-
 private:
     RefPtr<RealtimeMediaSource> m_audioSource;
     RefPtr<RealtimeMediaSource> m_videoSource;
index 035cca3..652866d 100644 (file)
@@ -60,13 +60,13 @@ private:
     OSStatus provideSourceDataNumOutputPackets(UInt32*, AudioBufferList*, AudioStreamPacketDescription**);
 
     dispatch_queue_t m_serialDispatchQueue;
-    CMTime m_lowWaterTime;
+    CMTime m_lowWaterTime { kCMTimeInvalid };
 
     RetainPtr<CMBufferQueueRef> m_outputBufferQueue;
     RetainPtr<CMBufferQueueRef> m_inputBufferQueue;
     bool m_isEncoding { false };
 
-    RetainPtr<AudioConverterRef> m_converter;
+    AudioConverterRef m_converter { nullptr };
     AudioStreamBasicDescription m_sourceFormat;
     AudioStreamBasicDescription m_destinationFormat;
     RetainPtr<CMFormatDescriptionRef> m_destinationFormatDescription;
@@ -74,9 +74,9 @@ private:
     UInt32 m_maxOutputPacketSize { 0 };
     Vector<AudioStreamPacketDescription> m_destinationPacketDescriptions;
 
-    CMTime m_currentNativePresentationTimeStamp;
-    CMTime m_currentOutputPresentationTimeStamp;
-    CMTime m_remainingPrimeDuration;
+    CMTime m_currentNativePresentationTimeStamp { kCMTimeInvalid };
+    CMTime m_currentOutputPresentationTimeStamp { kCMTimeInvalid };
+    CMTime m_remainingPrimeDuration { kCMTimeInvalid };
 
     Vector<char> m_sourceBuffer;
     Vector<char> m_destinationBuffer;
index c9bff64..6d04d8c 100644 (file)
@@ -58,6 +58,10 @@ AudioSampleBufferCompressor::AudioSampleBufferCompressor()
 AudioSampleBufferCompressor::~AudioSampleBufferCompressor()
 {
     dispatch_release(m_serialDispatchQueue);
+    if (m_converter) {
+        AudioConverterDispose(m_converter);
+        m_converter = nullptr;
+    }
 }
 
 bool AudioSampleBufferCompressor::initialize(CMBufferQueueTriggerCallback callback, void* callbackObject)
@@ -112,19 +116,19 @@ bool AudioSampleBufferCompressor::initAudioConverterForSourceFormatDescription(C
         RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor AudioConverterNew failed with %d", error);
         return false;
     }
-    m_converter = adoptCF(converter);
+    m_converter = converter;
 
     size_t cookieSize = 0;
     const void *cookie = CMAudioFormatDescriptionGetMagicCookie(formatDescription, &cookieSize);
     if (cookieSize) {
-        if (auto error = AudioConverterSetProperty(m_converter.get(), kAudioConverterDecompressionMagicCookie, (UInt32)cookieSize, cookie)) {
+        if (auto error = AudioConverterSetProperty(m_converter, kAudioConverterDecompressionMagicCookie, (UInt32)cookieSize, cookie)) {
             RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor setting kAudioConverterDecompressionMagicCookie failed with %d", error);
             return false;
         }
     }
 
     size = sizeof(m_sourceFormat);
-    if (auto error = AudioConverterGetProperty(m_converter.get(), kAudioConverterCurrentInputStreamDescription, &size, &m_sourceFormat)) {
+    if (auto error = AudioConverterGetProperty(m_converter, kAudioConverterCurrentInputStreamDescription, &size, &m_sourceFormat)) {
         RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor getting kAudioConverterCurrentInputStreamDescription failed with %d", error);
         return false;
     }
@@ -135,7 +139,7 @@ bool AudioSampleBufferCompressor::initAudioConverterForSourceFormatDescription(C
     }
 
     size = sizeof(m_destinationFormat);
-    if (auto error = AudioConverterGetProperty(m_converter.get(), kAudioConverterCurrentOutputStreamDescription, &size, &m_destinationFormat)) {
+    if (auto error = AudioConverterGetProperty(m_converter, kAudioConverterCurrentOutputStreamDescription, &size, &m_destinationFormat)) {
         RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor getting kAudioConverterCurrentOutputStreamDescription failed with %d", error);
         return false;
     }
@@ -149,7 +153,7 @@ bool AudioSampleBufferCompressor::initAudioConverterForSourceFormatDescription(C
             outputBitRate = 32000;
 
         size = sizeof(outputBitRate);
-        if (auto error = AudioConverterSetProperty(m_converter.get(), kAudioConverterEncodeBitRate, size, &outputBitRate)) {
+        if (auto error = AudioConverterSetProperty(m_converter, kAudioConverterEncodeBitRate, size, &outputBitRate)) {
             RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor setting kAudioConverterEncodeBitRate failed with %d", error);
             return false;
         }
@@ -159,7 +163,7 @@ bool AudioSampleBufferCompressor::initAudioConverterForSourceFormatDescription(C
         // If the destination format is VBR, we need to get max size per packet from the converter.
         size = sizeof(m_maxOutputPacketSize);
 
-        if (auto error = AudioConverterGetProperty(m_converter.get(), kAudioConverterPropertyMaximumOutputPacketSize, &size, &m_maxOutputPacketSize)) {
+        if (auto error = AudioConverterGetProperty(m_converter, kAudioConverterPropertyMaximumOutputPacketSize, &size, &m_maxOutputPacketSize)) {
             RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor getting kAudioConverterPropertyMaximumOutputPacketSize failed with %d", error);
             return false;
         }
@@ -189,7 +193,7 @@ void AudioSampleBufferCompressor::attachPrimingTrimsIfNeeded(CMSampleBufferRef b
         AudioConverterPrimeInfo primeInfo { 0, 0 };
         UInt32 size = sizeof(primeInfo);
 
-        if (auto error = AudioConverterGetProperty(m_converter.get(), kAudioConverterPrimeInfo, &size, &primeInfo)) {
+        if (auto error = AudioConverterGetProperty(m_converter, kAudioConverterPrimeInfo, &size, &primeInfo)) {
             RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor getting kAudioConverterPrimeInfo failed with %d", error);
             return;
         }
@@ -211,25 +215,25 @@ RetainPtr<NSNumber> AudioSampleBufferCompressor::gradualDecoderRefreshCount()
 {
     UInt32 delaySize = sizeof(uint32_t);
     uint32_t originalDelayMode = 0;
-    if (auto error = AudioConverterGetProperty(m_converter.get(), kAudioCodecPropertyDelayMode, &delaySize, &originalDelayMode)) {
+    if (auto error = AudioConverterGetProperty(m_converter, kAudioCodecPropertyDelayMode, &delaySize, &originalDelayMode)) {
         RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor getting kAudioCodecPropertyDelayMode failed with %d", error);
         return nil;
     }
 
     uint32_t optimalDelayMode = kAudioCodecDelayMode_Optimal;
-    if (auto error = AudioConverterSetProperty(m_converter.get(), kAudioCodecPropertyDelayMode, delaySize, &optimalDelayMode)) {
+    if (auto error = AudioConverterSetProperty(m_converter, kAudioCodecPropertyDelayMode, delaySize, &optimalDelayMode)) {
         RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor setting kAudioCodecPropertyDelayMode failed with %d", error);
         return nil;
     }
 
     UInt32 primeSize = sizeof(AudioCodecPrimeInfo);
     AudioCodecPrimeInfo primeInfo { 0, 0 };
-    if (auto error = AudioConverterGetProperty(m_converter.get(), kAudioCodecPropertyPrimeInfo, &primeSize, &primeInfo)) {
+    if (auto error = AudioConverterGetProperty(m_converter, kAudioCodecPropertyPrimeInfo, &primeSize, &primeInfo)) {
         RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor getting kAudioCodecPropertyPrimeInfo failed with %d", error);
         return nil;
     }
 
-    if (auto error = AudioConverterSetProperty(m_converter.get(), kAudioCodecPropertyDelayMode, delaySize, &originalDelayMode)) {
+    if (auto error = AudioConverterSetProperty(m_converter, kAudioCodecPropertyDelayMode, delaySize, &originalDelayMode)) {
         RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor setting kAudioCodecPropertyDelayMode failed with %d", error);
         return nil;
     }
@@ -242,11 +246,11 @@ CMSampleBufferRef AudioSampleBufferCompressor::sampleBufferWithNumPackets(UInt32
     if (!m_destinationFormatDescription) {
         UInt32 cookieSize = 0;
 
-        auto error = AudioConverterGetPropertyInfo(m_converter.get(), kAudioConverterCompressionMagicCookie, &cookieSize, NULL);
+        auto error = AudioConverterGetPropertyInfo(m_converter, kAudioConverterCompressionMagicCookie, &cookieSize, NULL);
         if ((error == noErr) && !!cookieSize) {
             cookie.resize(cookieSize);
 
-            if (auto error = AudioConverterGetProperty(m_converter.get(), kAudioConverterCompressionMagicCookie, &cookieSize, cookie.data())) {
+            if (auto error = AudioConverterGetProperty(m_converter, kAudioConverterCompressionMagicCookie, &cookieSize, cookie.data())) {
                 RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor getting kAudioConverterCompressionMagicCookie failed with %d", error);
                 return nil;
             }
@@ -439,7 +443,7 @@ void AudioSampleBufferCompressor::processSampleBuffersUntilLowWaterTime(CMTime l
         UInt32 outputPacketSize = m_destinationFormat.mBytesPerPacket ? m_destinationFormat.mBytesPerPacket : m_maxOutputPacketSize;
         UInt32 numOutputPackets = (UInt32)m_destinationBuffer.capacity() / outputPacketSize;
 
-        auto error = AudioConverterFillComplexBuffer(m_converter.get(), audioConverterComplexInputDataProc, this, &numOutputPackets, &fillBufferList, m_destinationPacketDescriptions.data());
+        auto error = AudioConverterFillComplexBuffer(m_converter, audioConverterComplexInputDataProc, this, &numOutputPackets, &fillBufferList, m_destinationPacketDescriptions.data());
         if (error) {
             RELEASE_LOG_ERROR(MediaStream, "AudioSampleBufferCompressor AudioConverterFillComplexBuffer failed with %d", error);
             return;
index b7a9d71..3da1b29 100644 (file)
@@ -114,8 +114,8 @@ private:
     RetainPtr<CMFormatDescriptionRef> m_videoFormatDescription;
     std::unique_ptr<VideoSampleBufferCompressor> m_videoCompressor;
     RetainPtr<AVAssetWriterInput> m_videoAssetWriterInput;
-    CMTime m_lastVideoPresentationTime;
-    CMTime m_lastVideoDecodingTime;
+    CMTime m_lastVideoPresentationTime { kCMTimeInvalid };
+    CMTime m_lastVideoDecodingTime { kCMTimeInvalid };
     bool m_hasEncodedVideoSamples { false };
 
     RetainPtr<WebAVAssetWriterDelegate> m_writerDelegate;
index 7f3087e..68f0254 100644 (file)
@@ -435,16 +435,18 @@ void MediaRecorderPrivateWriter::stopRecording()
     m_isStopping = true;
     // We hop to the main thread since finishing the video compressor might trigger starting the writer asynchronously.
     callOnMainThread([this, weakThis = makeWeakPtr(this)]() mutable {
-        auto whenFinished = [this] {
-            m_isStopping = false;
-            if (m_fetchDataCompletionHandler) {
-                auto buffer = WTFMove(m_data);
-                m_fetchDataCompletionHandler(WTFMove(buffer));
-            }
+        auto whenFinished = [this, weakThis] {
+            if (!weakThis)
+                return;
 
+            m_isStopping = false;
             m_isStopped = false;
             m_hasStartedWriting = false;
-            clear();
+
+            if (m_writer)
+                m_writer.clear();
+            if (m_fetchDataCompletionHandler)
+                m_fetchDataCompletionHandler(std::exchange(m_data, nullptr));
         };
 
         if (!m_hasStartedWriting) {
@@ -461,12 +463,8 @@ void MediaRecorderPrivateWriter::stopRecording()
             [m_writer flush];
             ALLOW_DEPRECATED_DECLARATIONS_END
 
-            [m_writer finishWritingWithCompletionHandler:[weakThis = WTFMove(weakThis), whenFinished = WTFMove(whenFinished)]() mutable {
-                callOnMainThread([weakThis = WTFMove(weakThis), whenFinished = WTFMove(whenFinished)]() mutable {
-                    if (!weakThis)
-                        return;
-                    whenFinished();
-                });
+            [m_writer finishWritingWithCompletionHandler:[whenFinished = WTFMove(whenFinished)]() mutable {
+                callOnMainThread(WTFMove(whenFinished));
             }];
         });
     });
index 185ad12..89a2a78 100644 (file)
@@ -56,6 +56,10 @@ VideoSampleBufferCompressor::VideoSampleBufferCompressor(CMVideoCodecType output
 VideoSampleBufferCompressor::~VideoSampleBufferCompressor()
 {
     dispatch_release(m_serialDispatchQueue);
+    if (m_vtSession) {
+        VTCompressionSessionInvalidate(m_vtSession.get());
+        m_vtSession = nullptr;
+    }
 }
 
 bool VideoSampleBufferCompressor::initialize(CMBufferQueueTriggerCallback callback, void* callbackObject)
index 56e7c94..0dd6d01 100644 (file)
@@ -1,3 +1,21 @@
+2020-06-25  Youenn Fablet  <youenn@apple.com>
+
+        MediaRecorder stopRecorder() returns empty Blob after first use
+        https://bugs.webkit.org/show_bug.cgi?id=212274
+        <rdar://problem/63601298>
+
+        Reviewed by Eric Carlson.
+
+        Update implementation to do initialization as part of startRecording.
+
+        * GPUProcess/webrtc/RemoteMediaRecorderManager.cpp:
+        (WebKit::RemoteMediaRecorderManager::releaseRecorder):
+        Remove ASSERT as recorder creation in WebProcess is always ok while creation in GPUProcess may fail and m_recorders may not be populated.
+        * WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:
+        (WebKit::MediaRecorderPrivate::MediaRecorderPrivate):
+        (WebKit::MediaRecorderPrivate::startRecording):
+        * WebProcess/GPU/webrtc/MediaRecorderPrivate.h:
+
 2020-06-24  James Savage  <james.savage@apple.com>
 
         Upstream iPadOS 13.0 multi-window support
index 7f4c66d..dc5b7b6 100644 (file)
@@ -64,7 +64,6 @@ void RemoteMediaRecorderManager::createRecorder(MediaRecorderIdentifier identifi
 
 void RemoteMediaRecorderManager::releaseRecorder(MediaRecorderIdentifier identifier)
 {
-    ASSERT(m_recorders.contains(identifier));
     m_recorders.remove(identifier);
 }
 
index f5ac318..668e675 100644 (file)
@@ -45,12 +45,17 @@ using namespace WebCore;
 
 MediaRecorderPrivate::MediaRecorderPrivate(MediaStreamPrivate& stream)
     : m_identifier(MediaRecorderIdentifier::generate())
+    , m_stream(makeRef(stream))
     , m_connection(WebProcess::singleton().ensureGPUProcessConnection().connection())
 {
+}
+
+void MediaRecorderPrivate::startRecording(ErrorCallback&& errorCallback)
+{
     // FIXME: we will need to implement support for multiple audio/video tracks
     // Currently we only choose the first track as the recorded track.
 
-    auto selectedTracks = MediaRecorderPrivate::selectTracks(stream);
+    auto selectedTracks = MediaRecorderPrivate::selectTracks(m_stream);
     if (selectedTracks.audioTrack) {
         m_ringBuffer = makeUnique<CARingBuffer>(makeUniqueRef<SharedRingBufferStorage>(this));
         m_recordedAudioTrackID = selectedTracks.audioTrack->id();
@@ -64,15 +69,20 @@ MediaRecorderPrivate::MediaRecorderPrivate(MediaStreamPrivate& stream)
         width = selectedTracks.videoTrack->settings().width();
     }
 
-    m_connection->sendWithAsyncReply(Messages::RemoteMediaRecorderManager::CreateRecorder { m_identifier, !!selectedTracks.audioTrack, width, height }, [this, weakThis = makeWeakPtr(this), audioTrack = makeRefPtr(selectedTracks.audioTrack), videoTrack = makeRefPtr(selectedTracks.videoTrack)](auto&& exception) {
-        if (!weakThis)
+    m_connection->sendWithAsyncReply(Messages::RemoteMediaRecorderManager::CreateRecorder { m_identifier, !!selectedTracks.audioTrack, width, height }, [this, weakThis = makeWeakPtr(this), audioTrack = makeRefPtr(selectedTracks.audioTrack), videoTrack = makeRefPtr(selectedTracks.videoTrack), errorCallback = WTFMove(errorCallback)](auto&& exception) mutable {
+        if (!weakThis) {
+            errorCallback({ });
+            return;
+        }
+        if (exception) {
+            errorCallback(Exception { exception->code, WTFMove(exception->message) });
             return;
-        if (exception)
-            return m_errorCallback(Exception { exception->code, WTFMove(exception->message) });
+        }
         if (audioTrack)
             setAudioSource(&audioTrack->source());
         if (videoTrack)
             setVideoSource(&videoTrack->source());
+        errorCallback({ });
     }, 0);
 }
 
index 3d7bee2..5267aba 100644 (file)
@@ -58,14 +58,16 @@ private:
     void videoSampleAvailable(WebCore::MediaSample&) final;
     void fetchData(CompletionHandler<void(RefPtr<WebCore::SharedBuffer>&&, const String& mimeType)>&&) final;
     void stopRecording() final;
+    void startRecording(ErrorCallback&&) final;
     void audioSamplesAvailable(const WTF::MediaTime&, const WebCore::PlatformAudioData&, const WebCore::AudioStreamDescription&, size_t) final;
 
     // SharedRingBufferStorage::Client
     void storageChanged(SharedMemory*);
 
     MediaRecorderIdentifier m_identifier;
-
+    Ref<MediaStreamPrivate> m_stream;
     Ref<IPC::Connection> m_connection;
+
     String m_recordedAudioTrackID;
     String m_recordedVideoTrackID;