Attachment 342927 Details for Bug 184911 – Patch

[patch] Patch

bug-184911-20180618145647.patch (text/plain), 17.44 KB, created by youenn fablet on 2018-06-18 05:56:48 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2018-06-18 05:56:48 PDT

Size: 17.44 KB

patch

obsolete

>Subversion Revision: 232923
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index d7be2d4937bab03e6e595fd06e31852505c7a076..f29c5300c88b6934e0c322e6cc2362c9cf9f0d4f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2018-06-18  Youenn Fablet  <youenn@apple.com>
>+
>+        RTCRtpSender.replaceTrack(null) ends current track
>+        https://bugs.webkit.org/show_bug.cgi?id=184911
>+        <rdar://problem/40758138>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Before the patch, when replacing the sender track by null, the previous track was stopped.
>+        Instead of doing that, the track now stays alive and it is the realtime source that is stopped.
>+        This ensures that the data is no longer sent while the track can still be used elsewhere.
>+
>+        Covered by updated and rebased tests.
>+
>+        * Modules/mediastream/PeerConnectionBackend.h:
>+        * Modules/mediastream/RTCPeerConnection.cpp:
>+        (WebCore::RTCPeerConnection::addTransceiver):
>+        (WebCore::RTCPeerConnection::enqueueReplaceTrackTask):
>+        (WebCore::RTCPeerConnection::replaceTrack):
>+        * Modules/mediastream/RTCPeerConnection.h:
>+        * Modules/mediastream/RTCRtpSender.cpp:
>+        (WebCore::RTCRtpSender::replaceTrack):
>+        * Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:
>+        (WebCore::updateTrackSource):
>+        (WebCore::LibWebRTCPeerConnectionBackend::replaceTrack):
>+        * Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.h:
>+
> 2018-06-17  Zalan Bujtas  <zalan@apple.com>
> 
>         Anonymous block collapsing can destroy the renderer's parent.
>diff --git a/Source/WebCore/Modules/mediastream/PeerConnectionBackend.h b/Source/WebCore/Modules/mediastream/PeerConnectionBackend.h
>index 0fa48cdab99c908a9c5159bad5aa7ef583465edb..08ca435ecbe4054e7ac991b89e4b60f659fbf51f 100644
>--- a/Source/WebCore/Modules/mediastream/PeerConnectionBackend.h
>+++ b/Source/WebCore/Modules/mediastream/PeerConnectionBackend.h
>@@ -101,7 +101,7 @@ public:
>     virtual Vector<RefPtr<MediaStream>> getRemoteStreams() const = 0;
> 
>     virtual Ref<RTCRtpReceiver> createReceiver(const String& transceiverMid, const String& trackKind, const String& trackId) = 0;
>-    virtual void replaceTrack(RTCRtpSender&, Ref<MediaStreamTrack>&&, DOMPromiseDeferred<void>&&) = 0;
>+    virtual void replaceTrack(RTCRtpSender&, RefPtr<MediaStreamTrack>&&, DOMPromiseDeferred<void>&&) = 0;
>     virtual void notifyAddedTrack(RTCRtpSender&) { }
>     virtual void notifyRemovedTrack(RTCRtpSender&) { }
> 
>diff --git a/Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp b/Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp
>index 165fa5dfd12b2c2dcdef73e4297107c32d8bb566..b87f62fab03b40c0e22875fcabfb3b66779a5845 100644
>--- a/Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp
>+++ b/Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp
>@@ -199,6 +199,7 @@ ExceptionOr<Ref<RTCRtpTransceiver>> RTCPeerConnection::addTransceiver(AddTransce
>     const String& trackKind = track->kind();
> 
>     auto sender = RTCRtpSender::create(WTFMove(track), Vector<String>(), *this);
>+    m_backend->notifyAddedTrack(sender);
>     return completeAddTransceiver(WTFMove(sender), init, trackId, trackKind);
> }
> 
>@@ -566,13 +567,20 @@ void RTCPeerConnection::fireEvent(Event& event)
>     dispatchEvent(event);
> }
> 
>-void RTCPeerConnection::enqueueReplaceTrackTask(RTCRtpSender& sender, Ref<MediaStreamTrack>&& withTrack, DOMPromiseDeferred<void>&& promise)
>+void RTCPeerConnection::enqueueReplaceTrackTask(RTCRtpSender& sender, RefPtr<MediaStreamTrack>&& withTrack, DOMPromiseDeferred<void>&& promise)
> {
>     scriptExecutionContext()->postTask([protectedThis = makeRef(*this), protectedSender = makeRef(sender), promise = WTFMove(promise), withTrack = WTFMove(withTrack)](ScriptExecutionContext&) mutable {
>         if (protectedThis->isClosed())
>             return;
>+
>+        if (!withTrack) {
>+            protectedSender->setTrackToNull();
>+            promise.resolve();
>+            return;
>+        }
>+
>         bool hasTrack = protectedSender->track();
>-        protectedSender->setTrack(WTFMove(withTrack));
>+        protectedSender->setTrack(withTrack.releaseNonNull());
>         if (!hasTrack)
>             protectedThis->m_backend->notifyAddedTrack(protectedSender.get());
>         promise.resolve();
>@@ -583,20 +591,12 @@ void RTCPeerConnection::replaceTrack(RTCRtpSender& sender, RefPtr<MediaStreamTra
> {
>     INFO_LOG(LOGIDENTIFIER);
> 
>-    if (!withTrack) {
>-        scriptExecutionContext()->postTask([protectedSender = makeRef(sender), promise = WTFMove(promise)](ScriptExecutionContext&) mutable {
>-            protectedSender->setTrackToNull();
>-            promise.resolve();
>-        });
>-        return;
>-    }
>-    
>-    if (!sender.track()) {
>+    if (!sender.track() && withTrack) {
>         enqueueReplaceTrackTask(sender, withTrack.releaseNonNull(), WTFMove(promise));
>         return;
>     }
> 
>-    m_backend->replaceTrack(sender, withTrack.releaseNonNull(), WTFMove(promise));
>+    m_backend->replaceTrack(sender, WTFMove(withTrack), WTFMove(promise));
> }
> 
> RTCRtpParameters RTCPeerConnection::getParameters(RTCRtpSender& sender) const
>diff --git a/Source/WebCore/Modules/mediastream/RTCPeerConnection.h b/Source/WebCore/Modules/mediastream/RTCPeerConnection.h
>index e99751d6ba136ab413670f9c3058da4e27fb6cf5..f46ac9a6036dfcbbef8b6956c6b62dfaaf71e5ee 100644
>--- a/Source/WebCore/Modules/mediastream/RTCPeerConnection.h
>+++ b/Source/WebCore/Modules/mediastream/RTCPeerConnection.h
>@@ -153,7 +153,7 @@ public:
>     void disableICECandidateFiltering() { m_backend->disableICECandidateFiltering(); }
>     void enableICECandidateFiltering() { m_backend->enableICECandidateFiltering(); }
> 
>-    void enqueueReplaceTrackTask(RTCRtpSender&, Ref<MediaStreamTrack>&&, DOMPromiseDeferred<void>&&);
>+    void enqueueReplaceTrackTask(RTCRtpSender&, RefPtr<MediaStreamTrack>&&, DOMPromiseDeferred<void>&&);
> 
>     void clearController() { m_controller = nullptr; }
> 
>diff --git a/Source/WebCore/Modules/mediastream/RTCRtpSender.cpp b/Source/WebCore/Modules/mediastream/RTCRtpSender.cpp
>index 93db3dbc2d46e90086054360c24c95d4382ae84b..18dc9bd64da3b1ea89c2c06cf3c99a99a02e0711 100644
>--- a/Source/WebCore/Modules/mediastream/RTCRtpSender.cpp
>+++ b/Source/WebCore/Modules/mediastream/RTCRtpSender.cpp
>@@ -89,9 +89,6 @@ void RTCRtpSender::replaceTrack(RefPtr<MediaStreamTrack>&& withTrack, DOMPromise
>         return;
>     }
> 
>-    if (!withTrack && m_track)
>-        m_track->stopTrack();
>-
>     m_backend->replaceTrack(*this, WTFMove(withTrack), WTFMove(promise));
> }
> 
>diff --git a/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp b/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp
>index c0b988695612bca28533474a20f535f3d5d354d0..aaf2441c5ebed3c44d5eaab7c54e080b690d6caf 100644
>--- a/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp
>+++ b/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp
>@@ -372,12 +372,23 @@ void LibWebRTCPeerConnectionBackend::addRemoteStream(Ref<MediaStream>&& mediaStr
>     m_remoteStreams.append(WTFMove(mediaStream));
> }
> 
>-void LibWebRTCPeerConnectionBackend::replaceTrack(RTCRtpSender& sender, Ref<MediaStreamTrack>&& track, DOMPromiseDeferred<void>&& promise)
>+template<typename Source>
>+static inline bool updateTrackSource(Source& source, MediaStreamTrack* track)
>+{
>+    if (!track) {
>+        source->stop();
>+        return true;
>+    }
>+    return source->setSource(track->privateTrack());
>+}
>+
>+void LibWebRTCPeerConnectionBackend::replaceTrack(RTCRtpSender& sender, RefPtr<MediaStreamTrack>&& track, DOMPromiseDeferred<void>&& promise)
> {
>     ASSERT(sender.track());
>+
>     auto* currentTrack = sender.track();
> 
>-    ASSERT(currentTrack->source().type() == track->source().type());
>+    ASSERT(!track || currentTrack->source().type() == track->source().type());
>     switch (currentTrack->source().type()) {
>     case RealtimeMediaSource::Type::None:
>         ASSERT_NOT_REACHED();
>@@ -386,7 +397,7 @@ void LibWebRTCPeerConnectionBackend::replaceTrack(RTCRtpSender& sender, Ref<Medi
>     case RealtimeMediaSource::Type::Audio: {
>         for (auto& audioSource : m_audioSources) {
>             if (&audioSource->source() == &currentTrack->privateTrack()) {
>-                if (!audioSource->setSource(track->privateTrack())) {
>+                if (!updateTrackSource(audioSource, track.get())) {
>                     promise.reject(InvalidModificationError);
>                     return;
>                 }
>@@ -400,7 +411,7 @@ void LibWebRTCPeerConnectionBackend::replaceTrack(RTCRtpSender& sender, Ref<Medi
>     case RealtimeMediaSource::Type::Video: {
>         for (auto& videoSource : m_videoSources) {
>             if (&videoSource->source() == &currentTrack->privateTrack()) {
>-                if (!videoSource->setSource(track->privateTrack())) {
>+                if (!updateTrackSource(videoSource, track.get())) {
>                     promise.reject(InvalidModificationError);
>                     return;
>                 }
>diff --git a/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.h b/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.h
>index 1b70675fb034bcdc8cb247f1b0d4e8fa12394451..83a0e20c92f372969ee3cb1a00723f0adb4c42f4 100644
>--- a/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.h
>+++ b/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.h
>@@ -73,7 +73,7 @@ private:
>     RefPtr<RTCSessionDescription> currentRemoteDescription() const final;
>     RefPtr<RTCSessionDescription> pendingRemoteDescription() const final;
> 
>-    void replaceTrack(RTCRtpSender&, Ref<MediaStreamTrack>&&, DOMPromiseDeferred<void>&&) final;
>+    void replaceTrack(RTCRtpSender&, RefPtr<MediaStreamTrack>&&, DOMPromiseDeferred<void>&&) final;
>     RTCRtpParameters getParameters(RTCRtpSender&) const final;
> 
>     void emulatePlatformEvent(const String&) final { }
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 410491dc411e29eb372e6437c5c6208b6b1636ce..a5a2d123feff7c894491d88cdba931286db3516c 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-06-18  Youenn Fablet  <youenn@apple.com>
>+
>+        RTCRtpSender.replaceTrack(null) ends current track
>+        https://bugs.webkit.org/show_bug.cgi?id=184911
>+        <rdar://problem/40758138>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added checks for readyState to ensure the track remains live.
>+        Split the main test into several tests to ease readability.
>+
>+        * webrtc/video-replace-track-to-null-expected.txt:
>+        * webrtc/video-replace-track-to-null.html:
>+
> 2018-06-18  Zan Dobersek  <zdobersek@igalia.com>
> 
>         Unreviewed WPE gardening. Adding platform-specific baselines for
>diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
>index 132c6b5aa501d348c45d1bec82e84d68acc6ecff..a95ef879e5c6d468f4eac0036983a5e12486ccae 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,3 +1,13 @@
>+2018-06-18  Youenn Fablet  <youenn@apple.com>
>+
>+        RTCRtpSender.replaceTrack(null) ends current track
>+        https://bugs.webkit.org/show_bug.cgi?id=184911
>+        <rdar://problem/40758138>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * web-platform-tests/webrtc/RTCRtpSender-replaceTrack-expected.txt:
>+
> 2018-06-16  Brendan McLoughlin  <brendan@bocoup.com>
> 
>         Sync additional web-platform-tests directories up to 197cdad
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCRtpSender-replaceTrack-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCRtpSender-replaceTrack-expected.txt
>index ee68e344d54997db295342b717e981b55ffc20f0..71f717fe1fe6af4689b36d7b1bf7f5ca23c0a6c9 100644
>--- a/LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCRtpSender-replaceTrack-expected.txt
>+++ b/LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCRtpSender-replaceTrack-expected.txt
>@@ -3,9 +3,9 @@ PASS Calling replaceTrack on closed connection should reject with InvalidStateEr
> PASS Calling replaceTrack with track of different kind should reject with TypeError 
> FAIL Calling replaceTrack on stopped sender should reject with InvalidStateError assert_unreached: Should have rejected: undefined Reached unreachable code
> PASS Calling replaceTrack on sender with null track and not set to session description should resolve with sender.track set to given track 
>-FAIL Calling replaceTrack on sender not set to session description should resolve with sender.track set to given track promise_test: Unhandled rejection with value: object "InvalidModificationError:  The object can not be modified in this way."
>+PASS Calling replaceTrack on sender not set to session description should resolve with sender.track set to given track 
> PASS Calling replaceTrack(null) on sender not set to session description should resolve with sender.track set to null 
> PASS Calling replaceTrack(null) on sender set to session description should resolve with sender.track set to null 
>-FAIL Calling replaceTrack on sender with stopped track and and set to session description should resolve with sender.track set to given track promise_test: Unhandled rejection with value: object "InvalidModificationError:  The object can not be modified in this way."
>-FAIL Calling replaceTrack on sender with similar track and and set to session description should resolve with sender.track set to new track promise_test: Unhandled rejection with value: object "InvalidModificationError:  The object can not be modified in this way."
>+PASS Calling replaceTrack on sender with stopped track and and set to session description should resolve with sender.track set to given track 
>+PASS Calling replaceTrack on sender with similar track and and set to session description should resolve with sender.track set to new track 
> 
>diff --git a/LayoutTests/webrtc/video-replace-track-to-null-expected.txt b/LayoutTests/webrtc/video-replace-track-to-null-expected.txt
>index e8cad01ccd336ae82b495aefc5778e524af5061e..5c60c54d1387c4117b06579778d0ff54c2fa63d7 100644
>--- a/LayoutTests/webrtc/video-replace-track-to-null-expected.txt
>+++ b/LayoutTests/webrtc/video-replace-track-to-null-expected.txt
>@@ -1,3 +1,6 @@
> 
>-PASS Stopping sending video using replaceTrack 
>+PASS Set-up test 
>+PASS Test that frame counter increases 
>+PASS Test replaceTrack with null track 
>+PASS Test that frame counter no longer increases 
> 
>diff --git a/LayoutTests/webrtc/video-replace-track-to-null.html b/LayoutTests/webrtc/video-replace-track-to-null.html
>index d0a7bd5dad484ad40a444b27777aa886112d99aa..b5e36b59afa624c5aac437c84085ab6eea4ad4ac 100644
>--- a/LayoutTests/webrtc/video-replace-track-to-null.html
>+++ b/LayoutTests/webrtc/video-replace-track-to-null.html
>@@ -59,41 +59,49 @@ async function testFrameEncodedStopped(connection, count, previousFramesNumber)
>     return testFrameEncodedStopped(connection, ++count, framesEncodedNumber);
> }
> 
>-promise_test((test) => {
>+var sender;
>+var sendingTrack;
>+var connection;
>+promise_test(async (test) => {
>     if (window.testRunner)
>         testRunner.setUserMediaPermission(true);
> 
>-    var sender;
>-    var frontStream;
>-    var connection;
>-    return navigator.mediaDevices.getUserMedia({ video: true }).then((stream) => {
>-        frontStream = stream;
>-    }).then(() => {
>-        return new Promise((resolve, reject) => {
>-            createConnections((firstConnection) => {
>-                connection = firstConnection;
>-                sender = firstConnection.addTrack(frontStream.getVideoTracks()[0], frontStream);
>-            }, (secondConnection) => {
>-                secondConnection.ontrack = (trackEvent) => {
>-                    resolve(trackEvent.streams[0]);
>-                };
>-            });
>-            setTimeout(() => reject("Test timed out"), 5000);
>+    const frontStream = await navigator.mediaDevices.getUserMedia({ video: true });
>+
>+    const remoteStream = await new Promise((resolve, reject) => {
>+        createConnections((firstConnection) => {
>+            connection = firstConnection;
>+            sender = firstConnection.addTrack(frontStream.getVideoTracks()[0], frontStream);
>+        }, (secondConnection) => {
>+            secondConnection.ontrack = (trackEvent) => {
>+                resolve(trackEvent.streams[0]);
>+            };
>         });
>-    }).then((remoteStream) => {
>-        video.srcObject = remoteStream;
>-        return video.play();
>-    }).then(() => {
>-        return testFrameEncodedStarted(connection);
>-    }).then(() => {
>-    }).then(() => {
>-        promise = sender.replaceTrack(null);
>-        assert_true(!!sender.track);
>-        return promise;
>-    }).then(() => {
>-        return testFrameEncodedStopped(connection);
>+        setTimeout(() => reject("Test timed out"), 5000);
>     });
>-}, "Stopping sending video using replaceTrack");
>+
>+    video.srcObject = remoteStream;
>+    await video.play();
>+}, "Set-up test");
>+
>+promise_test(async (test) => {
>+    await testFrameEncodedStarted(connection);
>+
>+    sendingTrack = sender.track;
>+    assert_equals(sendingTrack.readyState, "live");
>+}, "Test that frame counter increases");
>+
>+promise_test(async (test) => {
>+    promise = sender.replaceTrack(null);
>+    assert_true(!!sender.track);
>+    await promise;
>+    assert_equals(sender.track, null);
>+}, "Test replaceTrack with null track");
>+
>+promise_test(async (test) => {
>+    await testFrameEncodedStopped(connection);
>+    assert_equals(sendingTrack.readyState, "live");
>+}, "Test that frame counter no longer increases");
>         </script>
>     </body>
> </html>

Actions: View | Formatted Diff | Diff

Attachments on bug 184911: 342919 | 342920 | 342923 | 342927 | 342999