Changeset 278410 in webkit

Timestamp:

Jun 3, 2021, 10:25:28 AM (5 years ago)

Author:

youenn@apple.com

Message:

Cherry-pick webrtc fix to correctly handle audio track state in case of renegotiation
​https://bugs.webkit.org/show_bug.cgi?id=226577

Reviewed by Eric Carlson.

LayoutTests/imported/w3c:

• web-platform-tests/webrtc/receiver-track-live.https-expected.txt:

Source/ThirdParty/libwebrtc:

Cherry-pick ​https://webrtc.googlesource.com/src/+/c335b0e63bff56ca0fbfa617dee6a644c85df164%5E%21/.
We need to do small changes to peer_connection.cc given the upstream fix is based on a newer version
which has some code moved from peer_connection.cc to rtp_transmission_manager.cc.

• Source/webrtc/pc/audio_rtp_receiver.cc:
• Source/webrtc/pc/audio_rtp_receiver.h:
• Source/webrtc/pc/peer_connection.cc:
• Source/webrtc/pc/peer_connection_rtp_unittest.cc:
• Source/webrtc/pc/remote_audio_source.cc:
• Source/webrtc/pc/remote_audio_source.h:
• Source/webrtc/pc/rtp_sender_receiver_unittest.cc:

LayoutTests:

Update test now that we have the correct behavior.

• webrtc/receiver-track-should-stay-live-even-if-receiver-is-inactive.html:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/webrtc/receiver-track-live.https-expected.txt (modified) (1 diff)
• LayoutTests/webrtc/receiver-track-should-stay-live-even-if-receiver-is-inactive.html (modified) (2 diffs)
• Source/ThirdParty/libwebrtc/ChangeLog (modified) (1 diff)
• Source/ThirdParty/libwebrtc/Source/webrtc/pc/audio_rtp_receiver.cc (modified) (2 diffs)
• Source/ThirdParty/libwebrtc/Source/webrtc/pc/audio_rtp_receiver.h (modified) (1 diff)
• Source/ThirdParty/libwebrtc/Source/webrtc/pc/peer_connection.cc (modified) (2 diffs)
• Source/ThirdParty/libwebrtc/Source/webrtc/pc/peer_connection_rtp_unittest.cc (modified) (1 diff)
• Source/ThirdParty/libwebrtc/Source/webrtc/pc/remote_audio_source.cc (modified) (4 diffs)
• Source/ThirdParty/libwebrtc/Source/webrtc/pc/remote_audio_source.h (modified) (3 diffs)
• Source/ThirdParty/libwebrtc/Source/webrtc/pc/rtp_sender_receiver_unittest.cc (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-06-03  Youenn Fablet  <youenn@apple.com>
+
+        Cherry-pick webrtc fix to correctly handle audio track state in case of renegotiation
+        https://bugs.webkit.org/show_bug.cgi?id=226577
+
+        Reviewed by Eric Carlson.
+
+        Update test now that we have the correct behavior.
+
+        * webrtc/receiver-track-should-stay-live-even-if-receiver-is-inactive.html:
+
 2021-06-03  Alan Bujtas  <zalan@apple.com>
 

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2021-06-03  Youenn Fablet  <youenn@apple.com>
+
+        Cherry-pick webrtc fix to correctly handle audio track state in case of renegotiation
+        https://bugs.webkit.org/show_bug.cgi?id=226577
+
+        Reviewed by Eric Carlson.
+
+        * web-platform-tests/webrtc/receiver-track-live.https-expected.txt:
+
 2021-06-02  Alex Christensen  <achristensen@webkit.org>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/webrtc/receiver-track-live.https-expected.txt

@@ -2 +2 @@
 
 PASS Setup audio call
-FAIL Inactivate the audio transceiver assert_equals: expected "live" but got "ended"
-FAIL Reactivate the audio transceiver assert_equals: expected "live" but got "ended"
+PASS Inactivate the audio transceiver
+PASS Reactivate the audio transceiver
 PASS Clean-up
 

trunk/LayoutTests/webrtc/receiver-track-should-stay-live-even-if-receiver-is-inactive.html

@@ -47 +47 @@
         await pc1.setRemoteDescription(answer);
 
-        // FIXME: The track should be live but is ended due to a bug in our backend.
-        assert_equals(remoteTrack.readyState, "ended");
+        assert_equals(remoteTrack.readyState, "live");
     }, "Inactivate the audio transceiver");
 
@@ -60 +59 @@
         await pc1.setRemoteDescription(answer);
 
-        // FIXME: The track should be live but is ended due to a bug in our backend.
-        assert_equals(remoteTrack.readyState, "ended");
+        assert_equals(remoteTrack.readyState, "live");
     }, "Reactivate the audio transceiver");
     </script>

trunk/Source/ThirdParty/libwebrtc/ChangeLog

@@ +1 @@
+2021-06-03  Youenn Fablet  <youenn@apple.com>
+
+        Cherry-pick webrtc fix to correctly handle audio track state in case of renegotiation
+        https://bugs.webkit.org/show_bug.cgi?id=226577
+
+        Reviewed by Eric Carlson.
+
+        Cherry-pick https://webrtc.googlesource.com/src/+/c335b0e63bff56ca0fbfa617dee6a644c85df164%5E%21/.
+        We need to do small changes to peer_connection.cc given the upstream fix is based on a newer version
+        which has some code moved from peer_connection.cc to rtp_transmission_manager.cc.
+
+        * Source/webrtc/pc/audio_rtp_receiver.cc:
+        * Source/webrtc/pc/audio_rtp_receiver.h:
+        * Source/webrtc/pc/peer_connection.cc:
+        * Source/webrtc/pc/peer_connection_rtp_unittest.cc:
+        * Source/webrtc/pc/remote_audio_source.cc:
+        * Source/webrtc/pc/remote_audio_source.h:
+        * Source/webrtc/pc/rtp_sender_receiver_unittest.cc:
+
 2021-06-02  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/ThirdParty/libwebrtc/Source/webrtc/pc/audio_rtp_receiver.cc

@@ -31 +31 @@
 AudioRtpReceiver::AudioRtpReceiver(rtc::Thread* worker_thread,
                                    std::string receiver_id,
-                                   std::vector<std::string> stream_ids)
+                                   std::vector<std::string> stream_ids,
+                                   bool is_unified_plan)
     : AudioRtpReceiver(worker_thread,
                        receiver_id,
-                       CreateStreamsFromIds(std::move(stream_ids))) {}
+                       CreateStreamsFromIds(std::move(stream_ids)),
+                       is_unified_plan) {}
 
 AudioRtpReceiver::AudioRtpReceiver(
     rtc::Thread* worker_thread,
     const std::string& receiver_id,
-    const std::vector<rtc::scoped_refptr<MediaStreamInterface>>& streams)
+    const std::vector<rtc::scoped_refptr<MediaStreamInterface>>& streams,
+    bool is_unified_plan)
     : worker_thread_(worker_thread),
       id_(receiver_id),
-      source_(new rtc::RefCountedObject<RemoteAudioSource>(worker_thread)),
+      source_(new rtc::RefCountedObject<RemoteAudioSource>(
+          worker_thread,
+          is_unified_plan
+              ? RemoteAudioSource::OnAudioChannelGoneAction::kSurvive
+              : RemoteAudioSource::OnAudioChannelGoneAction::kEnd)),
       track_(AudioTrackProxyWithInternal<AudioTrack>::Create(
           rtc::Thread::Current(),
@@ -140 +147 @@
     return;
   }
+  source_->SetState(MediaSourceInterface::kEnded);
   if (media_channel_) {
     // Allow that SetOutputVolume fail. This is the normal case when the

trunk/Source/ThirdParty/libwebrtc/Source/webrtc/pc/audio_rtp_receiver.h

@@ -40 +40 @@
   AudioRtpReceiver(rtc::Thread* worker_thread,
                    std::string receiver_id,
-                   std::vector<std::string> stream_ids);
+                   std::vector<std::string> stream_ids,
+                   bool is_unified_plan);
   // TODO(https://crbug.com/webrtc/9480): Remove this when streams() is removed.
   AudioRtpReceiver(
       rtc::Thread* worker_thread,
       const std::string& receiver_id,
-      const std::vector<rtc::scoped_refptr<MediaStreamInterface>>& streams);
+      const std::vector<rtc::scoped_refptr<MediaStreamInterface>>& streams,
+      bool is_unified_plan);
   virtual ~AudioRtpReceiver();
 

trunk/Source/ThirdParty/libwebrtc/Source/webrtc/pc/peer_connection.cc

@@ -1211 +1211 @@
     receiver = RtpReceiverProxyWithInternal<RtpReceiverInternal>::Create(
         signaling_thread(), new AudioRtpReceiver(worker_thread(), receiver_id,
-                                                 std::vector<std::string>({})));
+                                                 std::vector<std::string>({}), IsUnifiedPlan()));
     NoteUsageEvent(UsageEvent::AUDIO_ADDED);
   } else {
@@ -2254 +2254 @@
   // the constructor taking stream IDs instead.
   auto* audio_receiver = new AudioRtpReceiver(
-      worker_thread(), remote_sender_info.sender_id, streams);
+      worker_thread(), remote_sender_info.sender_id, streams, IsUnifiedPlan());
   audio_receiver->SetMediaChannel(voice_media_channel());
   if (remote_sender_info.sender_id == kDefaultAudioSenderId) {

trunk/Source/ThirdParty/libwebrtc/Source/webrtc/pc/peer_connection_rtp_unittest.cc

@@ -780 +780 @@
   EXPECT_EQ(receivers[0]->streams()[1]->id(), kStreamId2);
 }
+TEST_F(PeerConnectionRtpTestUnifiedPlan, TracksDoNotEndWhenSsrcChanges) {
+  constexpr uint32_t kFirstMungedSsrc = 1337u;
+
+  auto caller = CreatePeerConnection();
+  auto callee = CreatePeerConnection();
+
+  // Caller offers to receive audio and video.
+  RtpTransceiverInit init;
+  init.direction = RtpTransceiverDirection::kRecvOnly;
+  caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init);
+  caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO, init);
+
+  // Callee wants to send audio and video tracks.
+  callee->AddTrack(callee->CreateAudioTrack("audio_track"), {});
+  callee->AddTrack(callee->CreateVideoTrack("video_track"), {});
+
+  // Do inittial offer/answer exchange.
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  ASSERT_TRUE(
+      caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+  ASSERT_EQ(caller->observer()->add_track_events_.size(), 2u);
+  ASSERT_EQ(caller->pc()->GetReceivers().size(), 2u);
+
+  // Do a follow-up offer/answer exchange where the SSRCs are modified.
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  auto answer = callee->CreateAnswer();
+  auto& contents = answer->description()->contents();
+  ASSERT_TRUE(!contents.empty());
+  for (size_t i = 0; i < contents.size(); ++i) {
+    auto& mutable_streams = contents[i].media_description()->mutable_streams();
+    ASSERT_EQ(mutable_streams.size(), 1u);
+    mutable_streams[0].ssrcs = {kFirstMungedSsrc + static_cast<uint32_t>(i)};
+  }
+  ASSERT_TRUE(
+      callee->SetLocalDescription(CloneSessionDescription(answer.get())));
+  ASSERT_TRUE(
+      caller->SetRemoteDescription(CloneSessionDescription(answer.get())));
+
+  // No furher track events should fire because we never changed direction, only
+  // SSRCs.
+  ASSERT_EQ(caller->observer()->add_track_events_.size(), 2u);
+  // We should have the same number of receivers as before.
+  auto receivers = caller->pc()->GetReceivers();
+  ASSERT_EQ(receivers.size(), 2u);
+  // The tracks are still alive.
+  EXPECT_EQ(receivers[0]->track()->state(),
+            MediaStreamTrackInterface::TrackState::kLive);
+  EXPECT_EQ(receivers[1]->track()->state(),
+            MediaStreamTrackInterface::TrackState::kLive);
+}
 
 // Tests that with Unified Plan if the the stream id changes for a track when

trunk/Source/ThirdParty/libwebrtc/Source/webrtc/pc/remote_audio_source.cc

@@ -52 +52 @@
 };
 
-RemoteAudioSource::RemoteAudioSource(rtc::Thread* worker_thread)
+RemoteAudioSource::RemoteAudioSource(
+    rtc::Thread* worker_thread,
+    OnAudioChannelGoneAction on_audio_channel_gone_action)
     : main_thread_(rtc::Thread::Current()),
       worker_thread_(worker_thread),
+      on_audio_channel_gone_action_(on_audio_channel_gone_action),
       state_(MediaSourceInterface::kLive) {
   RTC_DCHECK(main_thread_);
@@ -91 +94 @@
          : media_channel->SetDefaultRawAudioSink(nullptr);
   });
+}
+
+void RemoteAudioSource::SetState(SourceState new_state) {
+  if (state_ != new_state) {
+    state_ = new_state;
+    FireOnChanged();
+  }
 }
 
@@ -159 +169 @@
 
 void RemoteAudioSource::OnAudioChannelGone() {
+  if (on_audio_channel_gone_action_ != OnAudioChannelGoneAction::kEnd) {
+    return;
+  }
   // Called when the audio channel is deleted.  It may be the worker thread
   // in libjingle or may be a different worker thread.
@@ -173 +186 @@
   RTC_DCHECK(main_thread_->IsCurrent());
   sinks_.clear();
-  state_ = MediaSourceInterface::kEnded;
-  FireOnChanged();
+  SetState(MediaSourceInterface::kEnded);
   // Will possibly delete this RemoteAudioSource since it is reference counted
   // in the message.

trunk/Source/ThirdParty/libwebrtc/Source/webrtc/pc/remote_audio_source.h

@@ -35 +35 @@
                           rtc::MessageHandler {
  public:
-  explicit RemoteAudioSource(rtc::Thread* worker_thread);
+  // In Unified Plan, receivers map to m= sections and their tracks and sources
+  // survive SSRCs being reconfigured. The life cycle of the remote audio source
+  // is associated with the life cycle of the m= section, and thus even if an
+  // audio channel is destroyed the RemoteAudioSource should kSurvive.
+  //
+  // In Plan B however, remote audio sources map 1:1 with an SSRCs and if an
+  // audio channel is destroyed, the RemoteAudioSource should kEnd.
+  enum class OnAudioChannelGoneAction {
+    kSurvive,
+    kEnd,
+  };
+
+  explicit RemoteAudioSource(
+      rtc::Thread* worker_thread,
+      OnAudioChannelGoneAction on_audio_channel_gone_action);
 
   // Register and unregister remote audio source with the underlying media
@@ -43 +57 @@
   void Stop(cricket::VoiceMediaChannel* media_channel,
             absl::optional<uint32_t> ssrc);
+  void SetState(SourceState new_state);
 
   // MediaSourceInterface implementation.
@@ -69 +84 @@
   rtc::Thread* const main_thread_;
   rtc::Thread* const worker_thread_;
+  const OnAudioChannelGoneAction on_audio_channel_gone_action_;
   std::list<AudioObserver*> audio_observers_;
   Mutex sink_lock_;

trunk/Source/ThirdParty/libwebrtc/Source/webrtc/pc/rtp_sender_receiver_unittest.cc

@@ -290 +290 @@
       std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams = {}) {
     audio_rtp_receiver_ =
-        new AudioRtpReceiver(rtc::Thread::Current(), kAudioTrackId, streams);
+        new AudioRtpReceiver(rtc::Thread::Current(), kAudioTrackId, streams,
+                             /*is_unified_plan=*/true);
     audio_rtp_receiver_->SetMediaChannel(voice_media_channel_);
     audio_rtp_receiver_->SetupMediaChannel(kAudioSsrc);