Title: [281926] trunk
Revision
281926
Author
you...@apple.com
Date
2021-09-02 09:19:33 -0700 (Thu, 02 Sep 2021)

Log Message

Migrate LibWebRTCMediaEndpoint from OnRenegotiationNeeded to OnRenegotiationNeededEvent
https://bugs.webkit.org/show_bug.cgi?id=229562

Reviewed by Eric Carlson.

LayoutTests/imported/w3c:

* web-platform-tests/webrtc/RTCPeerConnection-setRemoteDescription-rollback-expected.txt:
We are making progress by going further in the test.

Source/WebCore:

OnRenegotiationNeededEvent is the more spec-aligned version of OnRenegotiationNeeded.
We switch to it and update to check for validity of the event by using the given eventId.
We can remove some state variables like m_negotiationNeeded given we use the backend with eventId to know whether we can send the event.

Covered by existing tests.

* Modules/mediastream/PeerConnectionBackend.cpp:
(WebCore::PeerConnectionBackend::markAsNeedingNegotiation):
* Modules/mediastream/PeerConnectionBackend.h:
* Modules/mediastream/RTCPeerConnection.cpp:
(WebCore::RTCPeerConnection::updateNegotiationNeededFlag):
(WebCore::RTCPeerConnection::chainOperation):
Update code to better align with spec steps.
* Modules/mediastream/RTCPeerConnection.h:
* Modules/mediastream/RTCRtpTransceiver.cpp:
(WebCore::RTCRtpTransceiver::stop):
* Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:
(WebCore::LibWebRTCMediaEndpoint::isNegotiationNeeded const):
(WebCore::LibWebRTCMediaEndpoint::OnNegotiationNeededEvent):
* Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:
* Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:
(WebCore::LibWebRTCPeerConnectionBackend::isNegotiationNeeded const):
* Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.h:
* testing/MockLibWebRTCPeerConnection.cpp:
(WebCore::MockLibWebRTCPeerConnection::AddTrack):
(WebCore::MockLibWebRTCPeerConnection::RemoveTrack):

Modified Paths

Diff

Modified: trunk/LayoutTests/imported/w3c/ChangeLog (281925 => 281926)


--- trunk/LayoutTests/imported/w3c/ChangeLog	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/LayoutTests/imported/w3c/ChangeLog	2021-09-02 16:19:33 UTC (rev 281926)
@@ -1,3 +1,13 @@
+2021-09-02  Youenn Fablet  <you...@apple.com>
+
+        Migrate LibWebRTCMediaEndpoint from OnRenegotiationNeeded to OnRenegotiationNeededEvent
+        https://bugs.webkit.org/show_bug.cgi?id=229562
+
+        Reviewed by Eric Carlson.
+
+        * web-platform-tests/webrtc/RTCPeerConnection-setRemoteDescription-rollback-expected.txt:
+        We are making progress by going further in the test.
+
 2021-09-02  Antti Koivisto  <an...@apple.com>
 
         [CSS Cascade Layers] Import @import WPT tests

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCPeerConnection-setRemoteDescription-rollback-expected.txt (281925 => 281926)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCPeerConnection-setRemoteDescription-rollback-expected.txt	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCPeerConnection-setRemoteDescription-rollback-expected.txt	2021-09-02 16:19:33 UTC (rev 281926)
@@ -10,8 +10,8 @@
 FAIL rollback of a remote offer should keep a transceiver created by addtrack promise_test: Unhandled rejection with value: object "InvalidStateError: Remote description type 3 is incompatible with current signaling state 2"
 FAIL rollback of a remote offer should keep a transceiver without tracks promise_test: Unhandled rejection with value: object "InvalidStateError: Remote description type 3 is incompatible with current signaling state 2"
 FAIL explicit rollback of local offer should remove transceivers and transport promise_test: Unhandled rejection with value: object "InvalidStateError: Local description type 3 is incompatible with current signaling state 1"
-FAIL when using addTransceiver, implicit rollback of a local offer should visit stable state, but not fire negotiationneeded until we settle in stable assert_true: There should be no negotiationneeded event right now expected true got false
-FAIL when using addTrack, implicit rollback of a local offer should visit stable state, but not fire negotiationneeded assert_true: There should be no negotiationneeded event in this test expected true got false
+FAIL when using addTransceiver, implicit rollback of a local offer should visit stable state, but not fire negotiationneeded until we settle in stable promise_test: Unhandled rejection with value: object "InvalidStateError: Remote description type 0 is incompatible with current signaling state 1"
+FAIL when using addTrack, implicit rollback of a local offer should visit stable state, but not fire negotiationneeded promise_test: Unhandled rejection with value: object "InvalidStateError: Remote description type 0 is incompatible with current signaling state 1"
 FAIL rollback of a remote offer to negotiated stable state should enable applying of a local offer promise_test: Unhandled rejection with value: object "InvalidStateError: Remote description type 3 is incompatible with current signaling state 2"
 FAIL rollback of a local offer to negotiated stable state should enable applying of a remote offer promise_test: Unhandled rejection with value: object "InvalidStateError: Local description type 3 is incompatible with current signaling state 1"
 FAIL rollback a local offer with audio direction change to negotiated stable state and then add video receiver promise_test: Unhandled rejection with value: object "InvalidStateError: Local description type 3 is incompatible with current signaling state 1"

Modified: trunk/Source/WebCore/ChangeLog (281925 => 281926)


--- trunk/Source/WebCore/ChangeLog	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/ChangeLog	2021-09-02 16:19:33 UTC (rev 281926)
@@ -1,3 +1,37 @@
+2021-09-02  Youenn Fablet  <you...@apple.com>
+
+        Migrate LibWebRTCMediaEndpoint from OnRenegotiationNeeded to OnRenegotiationNeededEvent
+        https://bugs.webkit.org/show_bug.cgi?id=229562
+
+        Reviewed by Eric Carlson.
+
+        OnRenegotiationNeededEvent is the more spec-aligned version of OnRenegotiationNeeded.
+        We switch to it and update to check for validity of the event by using the given eventId.
+        We can remove some state variables like m_negotiationNeeded given we use the backend with eventId to know whether we can send the event.
+
+        Covered by existing tests.
+
+        * Modules/mediastream/PeerConnectionBackend.cpp:
+        (WebCore::PeerConnectionBackend::markAsNeedingNegotiation):
+        * Modules/mediastream/PeerConnectionBackend.h:
+        * Modules/mediastream/RTCPeerConnection.cpp:
+        (WebCore::RTCPeerConnection::updateNegotiationNeededFlag):
+        (WebCore::RTCPeerConnection::chainOperation):
+        Update code to better align with spec steps.
+        * Modules/mediastream/RTCPeerConnection.h:
+        * Modules/mediastream/RTCRtpTransceiver.cpp:
+        (WebCore::RTCRtpTransceiver::stop):
+        * Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:
+        (WebCore::LibWebRTCMediaEndpoint::isNegotiationNeeded const):
+        (WebCore::LibWebRTCMediaEndpoint::OnNegotiationNeededEvent):
+        * Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:
+        * Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:
+        (WebCore::LibWebRTCPeerConnectionBackend::isNegotiationNeeded const):
+        * Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.h:
+        * testing/MockLibWebRTCPeerConnection.cpp:
+        (WebCore::MockLibWebRTCPeerConnection::AddTrack):
+        (WebCore::MockLibWebRTCPeerConnection::RemoveTrack):
+
 2021-09-02  Philippe Normand  <pnorm...@igalia.com>
 
         REGRESSION(r277763): [Debug][GLib] fast/mediastream timeouts

Modified: trunk/Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp (281925 => 281926)


--- trunk/Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp	2021-09-02 16:19:33 UTC (rev 281926)
@@ -577,15 +577,9 @@
     doStop();
 }
 
-void PeerConnectionBackend::markAsNeedingNegotiation()
+void PeerConnectionBackend::markAsNeedingNegotiation(uint32_t eventId)
 {
-    if (m_negotiationNeeded)
-        return;
-
-    m_negotiationNeeded = true;
-
-    if (m_peerConnection.signalingState() == RTCSignalingState::Stable)
-        m_peerConnection.scheduleNegotiationNeededEvent();
+    m_peerConnection.updateNegotiationNeededFlag(eventId);
 }
 
 ExceptionOr<Ref<RTCRtpSender>> PeerConnectionBackend::addTrack(MediaStreamTrack&, Vector<String>&&)

Modified: trunk/Source/WebCore/Modules/mediastream/PeerConnectionBackend.h (281925 => 281926)


--- trunk/Source/WebCore/Modules/mediastream/PeerConnectionBackend.h	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/Modules/mediastream/PeerConnectionBackend.h	2021-09-02 16:19:33 UTC (rev 281926)
@@ -124,9 +124,8 @@
     virtual ExceptionOr<Ref<RTCRtpTransceiver>> addTransceiver(const String&, const RTCRtpTransceiverInit&);
     virtual ExceptionOr<Ref<RTCRtpTransceiver>> addTransceiver(Ref<MediaStreamTrack>&&, const RTCRtpTransceiverInit&);
 
-    void markAsNeedingNegotiation();
-    bool isNegotiationNeeded() const { return m_negotiationNeeded; };
-    void clearNegotiationNeededState() { m_negotiationNeeded = false; };
+    void markAsNeedingNegotiation(uint32_t);
+    virtual bool isNegotiationNeeded(uint32_t) const = 0;
 
     virtual void emulatePlatformEvent(const String& action) = 0;
 
@@ -254,7 +253,6 @@
     Ref<const Logger> m_logger;
     const void* m_logIdentifier;
 #endif
-    bool m_negotiationNeeded { false };
     bool m_finishedGatheringCandidates { false };
     uint64_t m_waitingForMDNSRegistration { 0 };
 };

Modified: trunk/Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp (281925 => 281926)


--- trunk/Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp	2021-09-02 16:19:33 UTC (rev 281926)
@@ -738,22 +738,28 @@
         dispatchEventWhenFeasible(Event::create(eventNames().connectionstatechangeEvent, Event::CanBubble::No, Event::IsCancelable::No));
 }
 
-void RTCPeerConnection::scheduleNegotiationNeededEvent()
+void RTCPeerConnection::updateNegotiationNeededFlag(std::optional<uint32_t> eventId)
 {
-    if (m_hasPendingOperation) {
-        m_shouldFireNegotiationNeededOnceOperationChainIsEmpty = true;
-        return;
-    }
-    scriptExecutionContext()->postTask([this, protectedThis = makeRef(*this)](ScriptExecutionContext&) {
+    scriptExecutionContext()->postTask([this, protectedThis = makeRef(*this), eventId](auto&) mutable {
         if (isClosed())
             return;
+        if (!eventId) {
+            if (!m_negotiationNeededEventId)
+                return;
+            eventId = m_negotiationNeededEventId;
+        }
         if (m_hasPendingOperation) {
-            m_shouldFireNegotiationNeededOnceOperationChainIsEmpty = true;
+            m_negotiationNeededEventId = *eventId;
             return;
         }
-        if (!m_backend->isNegotiationNeeded())
+        if (signalingState() != RTCSignalingState::Stable) {
+            m_negotiationNeededEventId = *eventId;
             return;
-        m_backend->clearNegotiationNeededState();
+        }
+
+        if (!m_backend->isNegotiationNeeded(*eventId))
+            return;
+
         dispatchEventWhenFeasible(Event::create(eventNames().negotiationneededEvent, Event::CanBubble::No, Event::IsCancelable::No));
     });
 }
@@ -871,17 +877,15 @@
             return;
         }
 
-        if (m_operations.isEmpty()) {
-            m_hasPendingOperation = false;
-            if (m_shouldFireNegotiationNeededOnceOperationChainIsEmpty) {
-                m_shouldFireNegotiationNeededOnceOperationChainIsEmpty = false;
-                scheduleNegotiationNeededEvent();
-            }
+        if (!m_operations.isEmpty()) {
+            auto promiseOperation = m_operations.takeFirst();
+            promiseOperation.second(WTFMove(promiseOperation.first));
             return;
         }
 
-        auto promiseOperation = m_operations.takeFirst();
-        promiseOperation.second(WTFMove(promiseOperation.first));
+        m_hasPendingOperation = false;
+        if (m_negotiationNeededEventId)
+            updateNegotiationNeededFlag({ });
     });
 
     if (m_hasPendingOperation || !m_operations.isEmpty()) {

Modified: trunk/Source/WebCore/Modules/mediastream/RTCPeerConnection.h (281925 => 281926)


--- trunk/Source/WebCore/Modules/mediastream/RTCPeerConnection.h	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/Modules/mediastream/RTCPeerConnection.h	2021-09-02 16:19:33 UTC (rev 281926)
@@ -173,7 +173,7 @@
     void updateIceGatheringState(RTCIceGatheringState);
     void updateIceConnectionState(RTCIceConnectionState);
 
-    void scheduleNegotiationNeededEvent();
+    void updateNegotiationNeededFlag(std::optional<uint32_t>);
 
     void dispatchEventWhenFeasible(Ref<Event>&&);
 
@@ -263,7 +263,7 @@
     Vector<Function<void()>> m_pendingTasks;
     Deque<std::pair<Ref<DeferredPromise>, Function<void(Ref<DeferredPromise>&&)>>> m_operations;
     bool m_hasPendingOperation { false };
-    bool m_shouldFireNegotiationNeededOnceOperationChainIsEmpty { false };
+    std::optional<uint32_t> m_negotiationNeededEventId;
     Vector<Ref<RTCDtlsTransport>> m_dtlsTransports;
     Vector<Ref<RTCIceTransport>> m_iceTransports;
 };

Modified: trunk/Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp (281925 => 281926)


--- trunk/Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp	2021-09-02 16:19:33 UTC (rev 281926)
@@ -118,7 +118,7 @@
     if (m_backend)
         m_backend->stop();
 
-    m_connection->scheduleNegotiationNeededEvent();
+    // No need to call negotiation needed, it will be done by the backend itself.
     return { };
 }
 

Modified: trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp (281925 => 281926)


--- trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp	2021-09-02 16:19:33 UTC (rev 281926)
@@ -120,6 +120,11 @@
         m_rtcSocketFactory->resume();
 }
 
+bool LibWebRTCMediaEndpoint::isNegotiationNeeded(uint32_t eventId) const
+{
+    return m_backend ? m_backend->ShouldFireNegotiationNeededEvent(eventId) : false;
+}
+
 static inline const char* sessionDescriptionType(RTCSdpType sdpType)
 {
     switch (sdpType) {
@@ -558,12 +563,12 @@
     m_remoteStreamsFromRemoteTrack.clear();
 }
 
-void LibWebRTCMediaEndpoint::OnRenegotiationNeeded()
+void LibWebRTCMediaEndpoint::OnNegotiationNeededEvent(uint32_t eventId)
 {
-    callOnMainThread([protectedThis = makeRef(*this)] {
+    callOnMainThread([protectedThis = makeRef(*this), eventId] {
         if (protectedThis->isStopped())
             return;
-        protectedThis->m_peerConnectionBackend.markAsNeedingNegotiation();
+        protectedThis->m_peerConnectionBackend.markAsNeedingNegotiation(eventId);
     });
 }
 

Modified: trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h (281925 => 281926)


--- trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h	2021-09-02 16:19:33 UTC (rev 281926)
@@ -123,6 +123,8 @@
     void resume();
     LibWebRTCProvider::SuspendableSocketFactory* rtcSocketFactory() { return m_rtcSocketFactory.get(); }
 
+    bool isNegotiationNeeded(uint32_t) const;
+
 private:
     LibWebRTCMediaEndpoint(LibWebRTCPeerConnectionBackend&, LibWebRTCProvider&);
 
@@ -132,7 +134,7 @@
     void OnTrack(rtc::scoped_refptr<webrtc::RtpTransceiverInterface>) final;
     void OnRemoveTrack(rtc::scoped_refptr<webrtc::RtpReceiverInterface>) final;
 
-    void OnRenegotiationNeeded() final;
+    void OnNegotiationNeededEvent(uint32_t) final;
     void OnIceConnectionChange(webrtc::PeerConnectionInterface::IceConnectionState) final;
     void OnIceGatheringChange(webrtc::PeerConnectionInterface::IceGatheringState) final;
     void OnIceCandidate(const webrtc::IceCandidateInterface*) final;

Modified: trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp (281925 => 281926)


--- trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp	2021-09-02 16:19:33 UTC (rev 281926)
@@ -109,6 +109,11 @@
         factory->disableRelay();
 }
 
+bool LibWebRTCPeerConnectionBackend::isNegotiationNeeded(uint32_t eventId) const
+{
+    return m_endpoint->isNegotiationNeeded(eventId);
+}
+
 static inline webrtc::PeerConnectionInterface::BundlePolicy bundlePolicyfromConfiguration(const MediaEndpointConfiguration& configuration)
 {
     switch (configuration.bundlePolicy) {

Modified: trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.h (281925 => 281926)


--- trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.h	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.h	2021-09-02 16:19:33 UTC (rev 281926)
@@ -113,6 +113,7 @@
     void suspend() final;
     void resume() final;
     void disableICECandidateFiltering() final;
+    bool isNegotiationNeeded(uint32_t) const final;
 
     Ref<LibWebRTCMediaEndpoint> m_endpoint;
     bool m_isLocalDescriptionSet { false };

Modified: trunk/Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp (281925 => 281926)


--- trunk/Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp	2021-09-02 15:42:27 UTC (rev 281925)
+++ trunk/Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp	2021-09-02 16:19:33 UTC (rev 281926)
@@ -297,7 +297,7 @@
 webrtc::RTCErrorOr<rtc::scoped_refptr<webrtc::RtpSenderInterface>> MockLibWebRTCPeerConnection::AddTrack(rtc::scoped_refptr<webrtc::MediaStreamTrackInterface> track, const std::vector<std::string>& streamIds)
 {
     LibWebRTCProvider::callOnWebRTCSignalingThread([observer = &m_observer] {
-        observer->OnRenegotiationNeeded();
+        observer->OnNegotiationNeededEvent(0);
     });
 
     if (!streamIds.empty())
@@ -314,7 +314,7 @@
 bool MockLibWebRTCPeerConnection::RemoveTrack(webrtc::RtpSenderInterface* sender)
 {
     LibWebRTCProvider::callOnWebRTCSignalingThread([observer = &m_observer] {
-        observer->OnRenegotiationNeeded();
+        observer->OnNegotiationNeededEvent(0);
     });
     bool isRemoved = false;
     return m_transceivers.removeFirstMatching([&](auto& transceiver) {
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to