[Unified Plan] Implement FiredDirection for RtpTransceiver

Bug: webrtc:9236
Change-Id: Ib5a8215f3762f35b68d2a285c7d676f93f1212c5
Reviewed-on: https://webrtc-review.googlesource.com/88921
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24010}

api/rtptransceiverinterface.h

@@ -107,6 +107,15 @@ class RtpTransceiverInterface : public rtc::RefCountInterface {
   // https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-currentdirection
   virtual absl::optional<RtpTransceiverDirection> current_direction() const = 0;
 
+  // An internal slot designating for which direction the relevant
+  // PeerConnection events have been fired. This is to ensure that events like
+  // OnAddTrack only get fired once even if the same session description is
+  // applied again.
+  // Exposed in the public interface for use by Chromium.
+  virtual absl::optional<RtpTransceiverDirection> fired_direction() const {
+    return absl::nullopt;
+  }
+
   // The Stop method irreversibly stops the RtpTransceiver. The sender of this
   // transceiver will no longer send, the receiver will no longer receive.
   // https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-stop

pc/peerconnection.cc

@@ -2048,6 +2048,8 @@ RTCError PeerConnection::ApplyLocalDescription(
     if (!error.ok()) {
       return error;
     }
+    std::vector<rtc::scoped_refptr<RtpTransceiverInterface>> remove_list;
+    std::vector<rtc::scoped_refptr<MediaStreamInterface>> removed_streams;
     for (auto transceiver : transceivers_) {
       const ContentInfo* content =
           FindMediaSectionForTransceiver(transceiver, local_description());
@@ -2055,14 +2057,31 @@ RTCError PeerConnection::ApplyLocalDescription(
         continue;
       }
       const MediaContentDescription* media_desc = content->media_description();
+      // 2.2.7.1.6: If description is of type "answer" or "pranswer", then run
+      // the following steps:
       if (type == SdpType::kPrAnswer || type == SdpType::kAnswer) {
+        // 2.2.7.1.6.1: If direction is "sendonly" or "inactive", and
+        // transceiver's [[FiredDirection]] slot is either "sendrecv" or
+        // "recvonly", process the removal of a remote track for the media
+        // description, given transceiver, removeList, and muteTracks.
+        if (!RtpTransceiverDirectionHasRecv(media_desc->direction()) &&
+            (transceiver->internal()->fired_direction() &&
+             RtpTransceiverDirectionHasRecv(
+                 *transceiver->internal()->fired_direction()))) {
+          ProcessRemovalOfRemoteTrack(transceiver, &remove_list,
+                                      &removed_streams);
+        }
+        // 2.2.7.1.6.2: Set transceiver's [[CurrentDirection]] and
+        // [[FiredDirection]] slots to direction.
         transceiver->internal()->set_current_direction(media_desc->direction());
+        transceiver->internal()->set_fired_direction(media_desc->direction());
       }
-      if (content->rejected && !transceiver->stopped()) {
+    }
-        RTC_LOG(LS_INFO) << "Stopping transceiver for MID=" << content->name
+    for (auto transceiver : remove_list) {
-                         << " since the media section was rejected.";
+      observer_->OnRemoveTrack(transceiver->receiver());
-        transceiver->Stop();
+    }
-      }
+    for (auto stream : removed_streams) {
+      observer_->OnRemoveStream(stream);
     }
   } else {
     // Media channels will be created only when offer is set. These may use new
@@ -2380,8 +2399,7 @@ RTCError PeerConnection::ApplyRemoteDescription(
   if (IsUnifiedPlan()) {
     std::vector<rtc::scoped_refptr<RtpTransceiverInterface>>
         now_receiving_transceivers;
-    std::vector<rtc::scoped_refptr<RtpTransceiverInterface>>
+    std::vector<rtc::scoped_refptr<RtpTransceiverInterface>> remove_list;
-        no_longer_receiving_transceivers;
     std::vector<rtc::scoped_refptr<MediaStreamInterface>> added_streams;
     std::vector<rtc::scoped_refptr<MediaStreamInterface>> removed_streams;
     for (auto transceiver : transceivers_) {
@@ -2403,9 +2421,8 @@ RTCError PeerConnection::ApplyRemoteDescription(
         stream_ids = media_desc->streams()[0].stream_ids();
       }
       if (RtpTransceiverDirectionHasRecv(local_direction) &&
-          (!transceiver->current_direction() ||
+          (!transceiver->fired_direction() ||
-           !RtpTransceiverDirectionHasRecv(
+           !RtpTransceiverDirectionHasRecv(*transceiver->fired_direction()))) {
-               *transceiver->current_direction()))) {
         RTC_LOG(LS_INFO) << "Processing the addition of a new track for MID="
                          << content->name << " (added to "
                          << GetStreamIdsString(stream_ids) << ").";
@@ -2428,34 +2445,27 @@ RTCError PeerConnection::ApplyRemoteDescription(
         transceiver->internal()->receiver_internal()->SetStreams(media_streams);
         now_receiving_transceivers.push_back(transceiver);
       }
-      // If direction is sendonly or inactive, and transceiver's current
+      // 2.2.8.1.7: If direction is "sendonly" or "inactive", and transceiver's
-      // direction is neither sendonly nor inactive, process the removal of a
+      // [[FiredDirection]] slot is either "sendrecv" or "recvonly", process the
-      // remote track for the media description.
+      // removal of a remote track for the media description, given transceiver,
+      // removeList, and muteTracks.
       if (!RtpTransceiverDirectionHasRecv(local_direction) &&
-          (transceiver->current_direction() &&
+          (transceiver->fired_direction() &&
-           RtpTransceiverDirectionHasRecv(*transceiver->current_direction()))) {
+           RtpTransceiverDirectionHasRecv(*transceiver->fired_direction()))) {
-        RTC_LOG(LS_INFO) << "Processing the removal of a track for MID="
+        ProcessRemovalOfRemoteTrack(transceiver, &remove_list,
-                         << content->name;
+                                    &removed_streams);
-        std::vector<rtc::scoped_refptr<MediaStreamInterface>> media_streams =
-            transceiver->internal()->receiver_internal()->streams();
-        // This will remove the remote track from the streams.
-        transceiver->internal()->receiver_internal()->set_stream_ids({});
-        no_longer_receiving_transceivers.push_back(transceiver);
-        // Remove any streams that no longer have tracks.
-        // TODO(https://crbug.com/webrtc/9480): When we use stream IDs instead
-        // of streams, see if the stream was removed by checking if this was the
-        // last receiver with that stream ID.
-        for (auto stream : media_streams) {
-          if (stream->GetAudioTracks().empty() &&
-              stream->GetVideoTracks().empty()) {
-            remote_streams_->RemoveStream(stream);
-            removed_streams.push_back(stream);
-          }
-        }
       }
+      // 2.2.8.1.8: Set transceiver's [[FiredDirection]] slot to direction.
+      transceiver->internal()->set_fired_direction(local_direction);
+      // 2.2.8.1.9: If description is of type "answer" or "pranswer", then run
+      // the following steps:
       if (type == SdpType::kPrAnswer || type == SdpType::kAnswer) {
+        // 2.2.8.1.9.1: Set transceiver's [[CurrentDirection]] slot to
+        // direction.
         transceiver->internal()->set_current_direction(local_direction);
       }
+      // 2.2.8.1.10: If the media description is rejected, and transceiver is
+      // not already stopped, stop the RTCRtpTransceiver transceiver.
       if (content->rejected && !transceiver->stopped()) {
         RTC_LOG(LS_INFO) << "Stopping transceiver for MID=" << content->name
                          << " since the media section was rejected.";
@@ -2482,7 +2492,7 @@ RTCError PeerConnection::ApplyRemoteDescription(
     for (auto stream : added_streams) {
       observer_->OnAddStream(stream);
     }
-    for (auto transceiver : no_longer_receiving_transceivers) {
+    for (auto transceiver : remove_list) {
       observer_->OnRemoveTrack(transceiver->receiver());
     }
     for (auto stream : removed_streams) {
@@ -2574,6 +2584,31 @@ RTCError PeerConnection::ApplyRemoteDescription(
   return RTCError::OK();
 }
 
+void PeerConnection::ProcessRemovalOfRemoteTrack(
+    rtc::scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>>
+        transceiver,
+    std::vector<rtc::scoped_refptr<RtpTransceiverInterface>>* remove_list,
+    std::vector<rtc::scoped_refptr<MediaStreamInterface>>* removed_streams) {
+  RTC_DCHECK(transceiver->mid());
+  RTC_LOG(LS_INFO) << "Processing the removal of a track for MID="
+                   << *transceiver->mid();
+  std::vector<rtc::scoped_refptr<MediaStreamInterface>> media_streams =
+      transceiver->internal()->receiver_internal()->streams();
+  // This will remove the remote track from the streams.
+  transceiver->internal()->receiver_internal()->set_stream_ids({});
+  remove_list->push_back(transceiver);
+  // Remove any streams that no longer have tracks.
+  // TODO(https://crbug.com/webrtc/9480): When we use stream IDs instead
+  // of streams, see if the stream was removed by checking if this was the
+  // last receiver with that stream ID.
+  for (auto stream : media_streams) {
+    if (stream->GetAudioTracks().empty() && stream->GetVideoTracks().empty()) {
+      remote_streams_->RemoveStream(stream);
+      removed_streams->push_back(stream);
+    }
+  }
+}
+
 RTCError PeerConnection::UpdateTransceiversAndDataChannels(
     cricket::ContentSource source,
     const SessionDescriptionInterface& new_session,

pc/peerconnection.h

@@ -470,6 +470,20 @@ class PeerConnection : public PeerConnectionInternal,
           transceiver,
       const SessionDescriptionInterface* sdesc) const;
 
+  // Runs the algorithm **process the removal of a remote track** specified in
+  // the WebRTC specification.
+  // This method will update the following lists:
+  // |remove_list| is the list of transceivers for which the receiving track is
+  //     being removed.
+  // |removed_streams| is the list of streams which no longer have a receiving
+  //     track so should be removed.
+  // https://w3c.github.io/webrtc-pc/#process-remote-track-removal
+  void ProcessRemovalOfRemoteTrack(
+      rtc::scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>>
+          transceiver,
+      std::vector<rtc::scoped_refptr<RtpTransceiverInterface>>* remove_list,
+      std::vector<rtc::scoped_refptr<MediaStreamInterface>>* removed_streams);
+
   void OnNegotiationNeeded();
 
   bool IsClosed() const {

pc/peerconnection_rtp_unittest.cc

@@ -393,12 +393,9 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, SetDirectionHoldCallsOnTrackTwice) {
 }
 
 // Test that setting a remote offer twice with no answer in the middle results
-// in OnAddTrack being fired twice, once for each SetRemoteDescription. This
+// in OnAddTrack being fired only once.
-// happens since the RtpTransceiver's current_direction is only updated when
-// setting the answer.
-// TODO(bugs.webrtc.org/9236): This is spec-compliant but strange behavior.
 TEST_F(PeerConnectionRtpTestUnifiedPlan,
-       ApplyTwoOffersWithNoAnswerResultsInTwoAddTrackEvents) {
+       ApplyTwoRemoteOffersWithNoAnswerResultsInOneAddTrackEvent) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
@@ -408,17 +405,14 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan,
   ASSERT_EQ(1u, callee->observer()->add_track_events_.size());
 
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
-  EXPECT_EQ(2u, callee->observer()->add_track_events_.size());
+  EXPECT_EQ(1u, callee->observer()->add_track_events_.size());
 }
 
 // Test that setting a remote offer twice with no answer in the middle and the
 // track being removed between the two offers results in OnAddTrack being called
-// once the first time and OnRemoveTrack never getting called. This happens
+// once the first time and OnRemoveTrack being called once the second time.
-// since the RtpTransceiver's current_direction is only updated when setting the
-// answer.
-// TODO(bugs.webrtc.org/9236): This is spec-compliant but strange behavior.
 TEST_F(PeerConnectionRtpTestUnifiedPlan,
-       ApplyTwoOffersWithNoAnswerAndTrackRemovedResultsInNoRemoveTrackEvents) {
+       ApplyRemoteOfferAddThenRemoteOfferRemoveResultsInOneRemoveTrackEvent) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
@@ -426,12 +420,35 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan,
 
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
   ASSERT_EQ(1u, callee->observer()->add_track_events_.size());
+  EXPECT_EQ(0u, callee->observer()->remove_track_events_.size());
 
   caller->pc()->RemoveTrack(sender);
 
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+  EXPECT_EQ(1u, callee->observer()->add_track_events_.size());
+  EXPECT_EQ(1u, callee->observer()->remove_track_events_.size());
+}
+
+// Test that changing the direction from receiving to not receiving between
+// setting the remote offer and creating / setting the local answer results in
+// a remove track event when SetLocalDescription is called.
+TEST_F(PeerConnectionRtpTestUnifiedPlan,
+       ChangeDirectionInAnswerResultsInRemoveTrackEvent) {
+  auto caller = CreatePeerConnection();
+  caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
+  auto callee = CreatePeerConnection();
+  callee->AddAudioTrack("audio_track", {});
+
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
   EXPECT_EQ(1u, callee->observer()->add_track_events_.size());
   EXPECT_EQ(0u, callee->observer()->remove_track_events_.size());
+
+  auto callee_transceiver = callee->pc()->GetTransceivers()[0];
+  callee_transceiver->SetDirection(RtpTransceiverDirection::kSendOnly);
+
+  ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer()));
+  EXPECT_EQ(1u, callee->observer()->add_track_events_.size());
+  EXPECT_EQ(1u, callee->observer()->remove_track_events_.size());
 }
 
 // These tests examine the state of the peer connection as a result of
@@ -686,13 +703,16 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, UnsignaledSsrcCreatesReceiverStreams) {
 // Tests that with Unified Plan if the the stream id changes for a track when
 // when setting a new remote description, that the media stream is updated
 // appropriately for the receiver.
-TEST_F(PeerConnectionRtpTestUnifiedPlan, RemoteStreamIdChangesUpdatesReceiver) {
+// TODO(https://github.com/w3c/webrtc-pc/issues/1937): Resolve spec issue or fix
+// test.
+TEST_F(PeerConnectionRtpTestUnifiedPlan,
+       DISABLED_RemoteStreamIdChangesUpdatesReceiver) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
   const char kStreamId1[] = "stream1";
   const char kStreamId2[] = "stream2";
-  caller->AddTrack(caller->CreateAudioTrack("audio_track1"), {kStreamId1});
+  caller->AddAudioTrack("audio_track1", {kStreamId1});
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
   EXPECT_EQ(callee->observer()->add_track_events_.size(), 1u);
 
@@ -704,9 +724,9 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, RemoteStreamIdChangesUpdatesReceiver) {
   contents[0].media_description()->mutable_streams()[0].set_stream_ids(
       {kStreamId2});
 
-  // Set the remote description and verify that the stream was updated properly.
+  // Set the remote description and verify that the stream was updated
-  ASSERT_TRUE(
+  // properly.
-      callee->SetRemoteDescription(CloneSessionDescription(offer.get())));
+  ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer)));
   auto receivers = callee->pc()->GetReceivers();
   ASSERT_EQ(receivers.size(), 1u);
   ASSERT_EQ(receivers[0]->streams().size(), 1u);

pc/peerconnectioninterface_unittest.cc

@@ -3292,15 +3292,7 @@ TEST_P(PeerConnectionInterfaceTest, OnAddTrackCallback) {
 
   // Create and set the updated remote SDP.
   CreateAndSetRemoteOffer(kSdpStringWithStream1PlanB);
-  if (sdp_semantics_ == SdpSemantics::kPlanB) {
+  EXPECT_EQ(observer_.num_added_tracks_, 2);
-    EXPECT_EQ(observer_.num_added_tracks_, 2);
-  } else {
-    // With Unified Plan, OnAddTrack will fire every time SetRemoteDescription
-    // is called until the offer/answer exchange is complete. So in this case
-    // OnAddTrack is fired twice for the first audio track plus the one time
-    // for the video track.
-    EXPECT_EQ(observer_.num_added_tracks_, 3);
-  }
   EXPECT_EQ(observer_.last_added_track_label_, kVideoTracks[0]);
 }
 

pc/rtptransceiver.cc

@@ -184,6 +184,10 @@ void RtpTransceiver::set_current_direction(RtpTransceiverDirection direction) {
   }
 }
 
+void RtpTransceiver::set_fired_direction(RtpTransceiverDirection direction) {
+  fired_direction_ = direction;
+}
+
 bool RtpTransceiver::stopped() const {
   return stopped_;
 }
@@ -208,6 +212,11 @@ absl::optional<RtpTransceiverDirection> RtpTransceiver::current_direction()
   return current_direction_;
 }
 
+absl::optional<RtpTransceiverDirection> RtpTransceiver::fired_direction()
+    const {
+  return fired_direction_;
+}
+
 void RtpTransceiver::Stop() {
   for (auto sender : senders_) {
     sender->internal()->Stop();

pc/rtptransceiver.h

@@ -141,6 +141,11 @@ class RtpTransceiver final
   // transceiver has been set.
   void set_current_direction(RtpTransceiverDirection direction);
 
+  // Sets the fired direction for this transceiver. The fired direction is null
+  // until SetRemoteDescription is called or an answer is set (either local or
+  // remote).
+  void set_fired_direction(RtpTransceiverDirection direction);
+
   // According to JSEP rules for SetRemoteDescription, RtpTransceivers can be
   // reused only if they were added by AddTrack.
   void set_created_by_addtrack(bool created_by_addtrack) {
@@ -167,6 +172,7 @@ class RtpTransceiver final
   RtpTransceiverDirection direction() const override;
   void SetDirection(RtpTransceiverDirection new_direction) override;
   absl::optional<RtpTransceiverDirection> current_direction() const override;
+  absl::optional<RtpTransceiverDirection> fired_direction() const override;
   void Stop() override;
   void SetCodecPreferences(rtc::ArrayView<RtpCodecCapability> codecs) override;
 
@@ -184,6 +190,7 @@ class RtpTransceiver final
   bool stopped_ = false;
   RtpTransceiverDirection direction_ = RtpTransceiverDirection::kInactive;
   absl::optional<RtpTransceiverDirection> current_direction_;
+  absl::optional<RtpTransceiverDirection> fired_direction_;
   absl::optional<std::string> mid_;
   absl::optional<size_t> mline_index_;
   bool created_by_addtrack_ = false;
@@ -202,6 +209,7 @@ PROXY_CONSTMETHOD0(bool, stopped);
 PROXY_CONSTMETHOD0(RtpTransceiverDirection, direction);
 PROXY_METHOD1(void, SetDirection, RtpTransceiverDirection);
 PROXY_CONSTMETHOD0(absl::optional<RtpTransceiverDirection>, current_direction);
+PROXY_CONSTMETHOD0(absl::optional<RtpTransceiverDirection>, fired_direction);
 PROXY_METHOD0(void, Stop);
 PROXY_METHOD1(void, SetCodecPreferences, rtc::ArrayView<RtpCodecCapability>);
 END_PROXY_MAP();