Attachment 357350 Details for Bug 192720 – Patch

[patch] Patch

bug-192720-20181214153813.patch (text/plain), 18.55 KB, created by youenn fablet on 2018-12-14 15:38:06 PST

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2018-12-14 15:38:06 PST

Size: 18.55 KB

patch

obsolete

>Subversion Revision: 239205
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index f63519b5014d2695af8e0878c6f6d780b4d0308d..389525ff1bdc272fa8f2e7af7471db1f86a80873 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,39 @@
>+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 NOBODY (OOPS!).
>+
>+        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
>diff --git a/Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp b/Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp
>index a78d4be7ef990971f09d98e4dec536dbdd9b19b9..ee8741d64ccb458668793e61d8355fdd12a893eb 100644
>--- a/Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp
>+++ b/Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp
>@@ -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);
>diff --git a/Source/WebCore/Modules/mediarecorder/MediaRecorder.h b/Source/WebCore/Modules/mediarecorder/MediaRecorder.h
>index 8c5a6112c5386f46378c05e446ff27b6459e1b1f..e19953087095eb12a2506bf9017d73d8ec3df7e4 100644
>--- a/Source/WebCore/Modules/mediarecorder/MediaRecorder.h
>+++ b/Source/WebCore/Modules/mediarecorder/MediaRecorder.h
>@@ -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;
>     
>diff --git a/Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp b/Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp
>index 3b9522887617a49a1d1c8958cc40db2b5d876433..1b9dc5345b881eb36d96fce3582df0e54a1385ba 100644
>--- a/Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp
>+++ b/Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp
>@@ -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()
>diff --git a/Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.h b/Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.h
>index 2f923c247d6c24fe604f772dea68e50498e2026e..d4adb152bd4baef7ef7bf055f6d9a83d867e67eb 100644
>--- a/Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.h
>+++ b/Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.h
>@@ -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
>diff --git a/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h b/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h
>index 464745a3ac6666dddd61a8177c8781ccea68a6fb..b4a81ae44cf5e9bf68c0d9708d9d0f9c91562e46 100644
>--- a/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h
>+++ b/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h
>@@ -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;
>diff --git a/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm b/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm
>index 63432878258b752439ea053808caafcd197970aa..843a0fd9c3d4aa666ba481645e61d5684582dd6b 100644
>--- a/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm
>+++ b/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm
>@@ -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 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;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index dc4e0419abbed4699a6b81a2774f6f0a2f77a9e5..e957ee91f43e33ad4c81468429be7d5a3997e853 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        * http/wpt/mediarecorder/MediaRecorder-onremovetrack-expected.txt: Added.
>+        * http/wpt/mediarecorder/MediaRecorder-onremovetrack.html: Added.
>+
> 2018-12-13  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         Make HTMLConverter work across shadow boundaries
>diff --git a/LayoutTests/http/wpt/mediarecorder/MediaRecorder-onremovetrack-expected.txt b/LayoutTests/http/wpt/mediarecorder/MediaRecorder-onremovetrack-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..53d8e06925ff85571f69a66e919b46b838941762
>--- /dev/null
>+++ b/LayoutTests/http/wpt/mediarecorder/MediaRecorder-onremovetrack-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..3f72fd319707ac553e9e3b96c9fa4c04d28f1ade
>--- /dev/null
>+++ b/LayoutTests/http/wpt/mediarecorder/MediaRecorder-onremovetrack.html
>@@ -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>

Actions: View | Formatted Diff | Diff

Attachments on bug 192720: 357349 | 357350 | 357352