MediaRecorderPrivateAVFImpl should have a Ref<MediaRecorderPrivateWriter> as member
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Dec 2018 01:07:18 +0000 (01:07 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Dec 2018 01:07:18 +0000 (01:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192720

Reviewed by Eric Carlson.

Source/WebCore:

Make sure that MediaRecorderPrivateAVFImpl takes a Ref<MediaRecorderPrivateWriter> as member,
as the latter is a ref counted object.
Made some refactoring to return early in case of error.

Also made sure that in the case of a MediaRecorder stopped by a track removal in the recorded stream
the MediaRecorder will stop listening for its tracks.
Otherwise, the tracks will continue calling the MediaRecorder even after it is dead.

Test: http/wpt/mediarecorder/MediaRecorder-onremovetrack.html

* Modules/mediarecorder/MediaRecorder.cpp:
(WebCore::MediaRecorder::didAddOrRemoveTrack):
(WebCore::MediaRecorder::setNewRecordingState): Deleted.
* Modules/mediarecorder/MediaRecorder.h:
* platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp:
(WebCore::MediaRecorderPrivateAVFImpl::create):
(WebCore::MediaRecorderPrivateAVFImpl::MediaRecorderPrivateAVFImpl):
(WebCore::MediaRecorderPrivateAVFImpl::sampleBufferUpdated):
(WebCore::MediaRecorderPrivateAVFImpl::audioSamplesAvailable):
(WebCore::MediaRecorderPrivateAVFImpl::stopRecording):
(WebCore::MediaRecorderPrivateAVFImpl::fetchData):
* platform/mediarecorder/MediaRecorderPrivateAVFImpl.h:
* platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:
* platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:
(WebCore::MediaRecorderPrivateWriter::create):
(WebCore::MediaRecorderPrivateWriter::MediaRecorderPrivateWriter):
(WebCore::MediaRecorderPrivateWriter::appendAudioSampleBuffer):
(WebCore::MediaRecorderPrivateWriter::setupWriter): Deleted.

LayoutTests:

* http/wpt/mediarecorder/MediaRecorder-onremovetrack-expected.txt: Added.
* http/wpt/mediarecorder/MediaRecorder-onremovetrack.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/http/wpt/mediarecorder/MediaRecorder-onremovetrack-expected.txt [new file with mode: 0644]
LayoutTests/http/wpt/mediarecorder/MediaRecorder-onremovetrack.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/MediaRecorderPrivateAVFImpl.cpp
Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.h
Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h
Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm

index 8642cbf..a8c77ad 100644 (file)
@@ -1,3 +1,13 @@
+2018-12-14  Youenn Fablet  <youenn@apple.com>
+
+        MediaRecorderPrivateAVFImpl should have a Ref<MediaRecorderPrivateWriter> as member
+        https://bugs.webkit.org/show_bug.cgi?id=192720
+
+        Reviewed by Eric Carlson.
+
+        * http/wpt/mediarecorder/MediaRecorder-onremovetrack-expected.txt: Added.
+        * http/wpt/mediarecorder/MediaRecorder-onremovetrack.html: Added.
+
 2018-12-14  Matt Baker  <mattbaker@apple.com>
 
         Web Inspector: Cookies view should use model objects instead of raw payload data
diff --git a/LayoutTests/http/wpt/mediarecorder/MediaRecorder-onremovetrack-expected.txt b/LayoutTests/http/wpt/mediarecorder/MediaRecorder-onremovetrack-expected.txt
new file mode 100644 (file)
index 0000000..53d8e06
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS MediaRecorder should stop when track is removed 
+
diff --git a/LayoutTests/http/wpt/mediarecorder/MediaRecorder-onremovetrack.html b/LayoutTests/http/wpt/mediarecorder/MediaRecorder-onremovetrack.html
new file mode 100644 (file)
index 0000000..3f72fd3
--- /dev/null
@@ -0,0 +1,52 @@
+<!doctype html>
+<html>
+<head>
+    <title>MediaRecorder should stop when track is removed</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>
+<script src="../resources/gc.js"></script>
+<canvas id="canvas" width="200" height="200">
+</canvas>
+<script>
+const ac = new webkitAudioContext();
+const osc = ac.createOscillator();
+const dest = ac.createMediaStreamDestination();
+const audio = dest.stream;
+osc.connect(dest);
+
+function finishTest()
+{
+    gc();
+    setTimeout(() => {
+        done();
+        ac.close();
+    }, 100);
+}
+
+function removeTrack()
+{
+    audio.removeTrack(audio.getAudioTracks()[0]);
+    setTimeout(finishTest, 100);
+}
+
+function doTest()
+{
+    const recorder = new MediaRecorder(audio);
+
+    recorder.onerror = () => {
+        assert_equals(recorder.state, 'inactive', 'MediaRecorder is inactive');
+    };
+    recorder.start();
+    osc.start();
+    assert_equals(recorder.state, 'recording', 'MediaRecorder has been started successfully');
+    setTimeout(removeTrack, 100);
+}
+
+doTest();
+
+</script>
+</body>
+</html>
index 9a3a456..0472951 100644 (file)
@@ -1,5 +1,41 @@
 2018-12-14  Youenn Fablet  <youenn@apple.com>
 
+        MediaRecorderPrivateAVFImpl should have a Ref<MediaRecorderPrivateWriter> as member
+        https://bugs.webkit.org/show_bug.cgi?id=192720
+
+        Reviewed by Eric Carlson.
+
+        Make sure that MediaRecorderPrivateAVFImpl takes a Ref<MediaRecorderPrivateWriter> as member,
+        as the latter is a ref counted object.
+        Made some refactoring to return early in case of error.
+
+        Also made sure that in the case of a MediaRecorder stopped by a track removal in the recorded stream
+        the MediaRecorder will stop listening for its tracks.
+        Otherwise, the tracks will continue calling the MediaRecorder even after it is dead.
+
+        Test: http/wpt/mediarecorder/MediaRecorder-onremovetrack.html
+
+        * Modules/mediarecorder/MediaRecorder.cpp:
+        (WebCore::MediaRecorder::didAddOrRemoveTrack):
+        (WebCore::MediaRecorder::setNewRecordingState): Deleted.
+        * Modules/mediarecorder/MediaRecorder.h:
+        * platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp:
+        (WebCore::MediaRecorderPrivateAVFImpl::create):
+        (WebCore::MediaRecorderPrivateAVFImpl::MediaRecorderPrivateAVFImpl):
+        (WebCore::MediaRecorderPrivateAVFImpl::sampleBufferUpdated):
+        (WebCore::MediaRecorderPrivateAVFImpl::audioSamplesAvailable):
+        (WebCore::MediaRecorderPrivateAVFImpl::stopRecording):
+        (WebCore::MediaRecorderPrivateAVFImpl::fetchData):
+        * platform/mediarecorder/MediaRecorderPrivateAVFImpl.h:
+        * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:
+        * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:
+        (WebCore::MediaRecorderPrivateWriter::create):
+        (WebCore::MediaRecorderPrivateWriter::MediaRecorderPrivateWriter):
+        (WebCore::MediaRecorderPrivateWriter::appendAudioSampleBuffer):
+        (WebCore::MediaRecorderPrivateWriter::setupWriter): Deleted.
+
+2018-12-14  Youenn Fablet  <youenn@apple.com>
+
         getSenders/getReceivers() should not return closed transceiver senders/receivers
         https://bugs.webkit.org/show_bug.cgi?id=192706
 
index a78d4be..ee8741d 100644 (file)
@@ -161,8 +161,9 @@ void MediaRecorder::didAddOrRemoveTrack()
     scheduleDeferredTask([this] {
         if (!m_isActive || state() == RecordingState::Inactive)
             return;
+        stopRecordingInternal();
         auto event = MediaRecorderErrorEvent::create(eventNames().errorEvent, Exception { UnknownError, "Track cannot be added to or removed from the MediaStream while recording is happening"_s });
-        setNewRecordingState(RecordingState::Inactive, WTFMove(event));
+        dispatchEvent(WTFMove(event));
     });
 }
 
@@ -187,12 +188,6 @@ void MediaRecorder::audioSamplesAvailable(MediaStreamTrackPrivate& track, const
     m_private->audioSamplesAvailable(track, mediaTime, audioData, description, sampleCount);
 }
 
-void MediaRecorder::setNewRecordingState(RecordingState newState, Ref<Event>&& event)
-{
-    m_state = newState;
-    dispatchEvent(WTFMove(event));
-}
-
 void MediaRecorder::scheduleDeferredTask(Function<void()>&& function)
 {
     ASSERT(function);
index 8c5a611..e199530 100644 (file)
@@ -103,7 +103,6 @@ private:
     void audioSamplesAvailable(MediaStreamTrackPrivate&, const MediaTime&, const PlatformAudioData&, const AudioStreamDescription&, size_t) final;
     
     void scheduleDeferredTask(Function<void()>&&);
-    void setNewRecordingState(RecordingState, Ref<Event>&&);
     
     static creatorFunction m_customCreator;
     
index 3b95228..1b9dc53 100644 (file)
@@ -38,58 +38,55 @@ namespace WebCore {
 
 std::unique_ptr<MediaRecorderPrivateAVFImpl> MediaRecorderPrivateAVFImpl::create(const MediaStreamPrivate& stream)
 {
-    auto instance = std::unique_ptr<MediaRecorderPrivateAVFImpl>(new MediaRecorderPrivateAVFImpl(stream));
-    if (!instance->m_isWriterReady)
-        return nullptr;
-    return instance;
-}
+    // FIXME: we will need to implement support for multiple audio/video tracks
+    // Currently we only choose the first track as the recorded track.
+    // FIXME: We would better to throw an exception to JavaScript if writer creation fails.
 
-MediaRecorderPrivateAVFImpl::MediaRecorderPrivateAVFImpl(const MediaStreamPrivate& stream)
-{
-    if (!m_writer.setupWriter())
-        return;
-    auto tracks = stream.tracks();
-    bool videoSelected = false;
-    bool audioSelected = false;
-    for (auto& track : tracks) {
+    String audioTrackId;
+    String videoTrackId;
+    const MediaStreamTrackPrivate* audioTrack { nullptr };
+    const MediaStreamTrackPrivate* videoTrack { nullptr };
+    for (auto& track : stream.tracks()) {
         if (!track->enabled() || track->ended())
             continue;
         switch (track->type()) {
         case RealtimeMediaSource::Type::Video: {
             auto& settings = track->settings();
-            if (videoSelected || !settings.supportsWidth() || !settings.supportsHeight())
-                break;
-            // FIXME: we will need to implement support for multiple video tracks, currently we only choose the first track as the recorded track.
-            // FIXME: we would better to throw an exception to JavaScript if setVideoInput failed
-            if (!m_writer.setVideoInput(settings.width(), settings.height()))
-                return;
-            m_recordedVideoTrackID = track->id();
-            videoSelected = true;
+            if (!videoTrack && settings.supportsWidth() && settings.supportsHeight()) {
+                videoTrack = track.get();
+                videoTrackId = videoTrack->id();
+            }
             break;
         }
-        case RealtimeMediaSource::Type::Audio: {
-            if (audioSelected)
-                break;
-            // FIXME: we will need to implement support for multiple audio tracks, currently we only choose the first track as the recorded track.
-            // FIXME: we would better to throw an exception to JavaScript if setAudioInput failed
-            if (!m_writer.setAudioInput())
-                return;
-            m_recordedAudioTrackID = track->id();
-            audioSelected = true;
+        case RealtimeMediaSource::Type::Audio:
+            if (!audioTrack) {
+                audioTrack = track.get();
+                audioTrackId = audioTrack->id();
+            }
             break;
-        }
         case RealtimeMediaSource::Type::None:
             break;
         }
     }
-    m_isWriterReady = true;
+    auto writer = MediaRecorderPrivateWriter::create(audioTrack, videoTrack);
+    if (!writer)
+        return nullptr;
+
+    return std::make_unique<MediaRecorderPrivateAVFImpl>(writer.releaseNonNull(), WTFMove(audioTrackId), WTFMove(videoTrackId));
+}
+
+MediaRecorderPrivateAVFImpl::MediaRecorderPrivateAVFImpl(Ref<MediaRecorderPrivateWriter>&& writer, String&& audioTrackId, String&& videoTrackId)
+    : m_writer(WTFMove(writer))
+    , m_recordedAudioTrackID(WTFMove(audioTrackId))
+    , m_recordedVideoTrackID(WTFMove(videoTrackId))
+{
 }
 
 void MediaRecorderPrivateAVFImpl::sampleBufferUpdated(MediaStreamTrackPrivate& track, MediaSample& sampleBuffer)
 {
     if (track.id() != m_recordedVideoTrackID)
         return;
-    m_writer.appendVideoSampleBuffer(sampleBuffer.platformSample().sample.cmSampleBuffer);
+    m_writer->appendVideoSampleBuffer(sampleBuffer.platformSample().sample.cmSampleBuffer);
 }
 
 void MediaRecorderPrivateAVFImpl::audioSamplesAvailable(MediaStreamTrackPrivate& track, const WTF::MediaTime& mediaTime, const PlatformAudioData& data, const AudioStreamDescription& description, size_t sampleCount)
@@ -98,17 +95,17 @@ void MediaRecorderPrivateAVFImpl::audioSamplesAvailable(MediaStreamTrackPrivate&
         return;
     ASSERT(is<WebAudioBufferList>(data));
     ASSERT(description.platformDescription().type == PlatformDescription::CAAudioStreamBasicType);
-    m_writer.appendAudioSampleBuffer(data, description, mediaTime, sampleCount);
+    m_writer->appendAudioSampleBuffer(data, description, mediaTime, sampleCount);
 }
 
 void MediaRecorderPrivateAVFImpl::stopRecording()
 {
-    m_writer.stopRecording();
+    m_writer->stopRecording();
 }
 
 RefPtr<SharedBuffer> MediaRecorderPrivateAVFImpl::fetchData()
 {
-    return m_writer.fetchData();
+    return m_writer->fetchData();
 }
 
 const String& MediaRecorderPrivateAVFImpl::mimeType()
index 2f923c2..d4adb15 100644 (file)
@@ -38,7 +38,9 @@ public:
     static std::unique_ptr<MediaRecorderPrivateAVFImpl> create(const MediaStreamPrivate&);
 
 private:
-    explicit MediaRecorderPrivateAVFImpl(const MediaStreamPrivate&);
+    MediaRecorderPrivateAVFImpl(Ref<MediaRecorderPrivateWriter>&&, String&& audioTrackId, String&& videoTrackId);
+
+    friend std::unique_ptr<MediaRecorderPrivateAVFImpl> std::make_unique<MediaRecorderPrivateAVFImpl>(Ref<MediaRecorderPrivateWriter>&&, String&&, String&&);
 
     void sampleBufferUpdated(MediaStreamTrackPrivate&, MediaSample&) final;
     void audioSamplesAvailable(MediaStreamTrackPrivate&, const WTF::MediaTime&, const PlatformAudioData&, const AudioStreamDescription&, size_t) final;
@@ -46,12 +48,9 @@ private:
     const String& mimeType() final;
     void stopRecording();
     
-    String m_recordedVideoTrackID;
+    Ref<MediaRecorderPrivateWriter> m_writer;
     String m_recordedAudioTrackID;
-
-    MediaRecorderPrivateWriter m_writer;
-    
-    bool m_isWriterReady { false };
+    String m_recordedVideoTrackID;
 };
 
 } // namespace WebCore
index 464745a..b4a81ae 100644 (file)
@@ -45,11 +45,12 @@ class MediaTime;
 namespace WebCore {
 
 class AudioStreamDescription;
+class MediaStreamTrackPrivate;
 class PlatformAudioData;
 
 class MediaRecorderPrivateWriter : public ThreadSafeRefCounted<MediaRecorderPrivateWriter, WTF::DestructionThread::Main>, public CanMakeWeakPtr<MediaRecorderPrivateWriter> {
 public:
-    MediaRecorderPrivateWriter() = default;
+    static RefPtr<MediaRecorderPrivateWriter> create(const MediaStreamTrackPrivate* audioTrack, const MediaStreamTrackPrivate* videoTrack);
     ~MediaRecorderPrivateWriter();
     
     bool setupWriter();
@@ -61,6 +62,7 @@ public:
     RefPtr<SharedBuffer> fetchData();
     
 private:
+    MediaRecorderPrivateWriter(RetainPtr<AVAssetWriter>&&, String&& path);
     void clear();
 
     RetainPtr<AVAssetWriter> m_writer;
index 6343287..57aeec3 100644 (file)
@@ -31,6 +31,7 @@
 #include "AudioStreamDescription.h"
 #include "FileSystem.h"
 #include "Logging.h"
+#include "MediaStreamTrackPrivate.h"
 #include "WebAudioBufferList.h"
 #include <AVFoundation/AVAssetWriter.h>
 #include <AVFoundation/AVAssetWriterInput.h>
@@ -86,6 +87,41 @@ namespace WebCore {
 
 using namespace PAL;
 
+RefPtr<MediaRecorderPrivateWriter> MediaRecorderPrivateWriter::create(const MediaStreamTrackPrivate* audioTrack, const MediaStreamTrackPrivate* videoTrack)
+{
+    NSString *directory = FileSystem::createTemporaryDirectory(@"videos");
+    NSString *filename = [NSString stringWithFormat:@"/%lld.mp4", CMClockGetTime(CMClockGetHostTimeClock()).value];
+    NSString *path = [directory stringByAppendingString:filename];
+
+    NSURL *outputURL = [NSURL fileURLWithPath:path];
+    String filePath = [path UTF8String];
+    NSError *error = nil;
+    auto avAssetWriter = adoptNS([allocAVAssetWriterInstance() initWithURL:outputURL fileType:AVFileTypeMPEG4 error:&error]);
+    if (error) {
+        RELEASE_LOG_ERROR(MediaStream, "create AVAssetWriter instance failed with error code %ld", (long)error.code);
+        return nullptr;
+    }
+
+    auto writer = adoptRef(*new MediaRecorderPrivateWriter(WTFMove(avAssetWriter), WTFMove(filePath)));
+
+    if (audioTrack && !writer->setAudioInput())
+        return nullptr;
+
+    if (videoTrack) {
+        auto& settings = videoTrack->settings();
+        if (!writer->setVideoInput(settings.width(), settings.height()))
+            return nullptr;
+    }
+
+    return WTFMove(writer);
+}
+
+MediaRecorderPrivateWriter::MediaRecorderPrivateWriter(RetainPtr<AVAssetWriter>&& avAssetWriter, String&& filePath)
+    : m_writer(WTFMove(avAssetWriter))
+    , m_path(WTFMove(filePath))
+{
+}
+
 MediaRecorderPrivateWriter::~MediaRecorderPrivateWriter()
 {
     clear();
@@ -105,26 +141,6 @@ void MediaRecorderPrivateWriter::clear()
         m_writer.clear();
 }
 
-bool MediaRecorderPrivateWriter::setupWriter()
-{
-    ASSERT(!m_writer);
-    
-    NSString *directory = FileSystem::createTemporaryDirectory(@"videos");
-    NSString *filename = [NSString stringWithFormat:@"/%lld.mp4", CMClockGetTime(CMClockGetHostTimeClock()).value];
-    NSString *path = [directory stringByAppendingString:filename];
-
-    NSURL *outputURL = [NSURL fileURLWithPath:path];
-    m_path = [path UTF8String];
-    NSError *error = nil;
-    m_writer = adoptNS([allocAVAssetWriterInstance() initWithURL:outputURL fileType:AVFileTypeMPEG4 error:&error]);
-    if (error) {
-        RELEASE_LOG_ERROR(MediaStream, "create AVAssetWriter instance failed with error code %ld", (long)error.code);
-        m_writer = nullptr;
-        return false;
-    }
-    return true;
-}
-
 bool MediaRecorderPrivateWriter::setVideoInput(int width, int height)
 {
     ASSERT(!m_videoInput);
@@ -260,7 +276,7 @@ void MediaRecorderPrivateWriter::appendAudioSampleBuffer(const PlatformAudioData
         }
         m_isFirstAudioSample = false;
         RefPtr<MediaRecorderPrivateWriter> protectedThis = this;
-        [m_audioInput requestMediaDataWhenReadyOnQueue:m_audioPullQueue usingBlock:[this, protectedThis] {
+        [m_audioInput requestMediaDataWhenReadyOnQueue:m_audioPullQueue usingBlock:[this, protectedThis = WTFMove(protectedThis)] {
             do {
                 if (![m_audioInput isReadyForMoreMediaData])
                     break;