Restore FiredDirection and maybe fire OnTrack in Rollback.

Prior to this CL, rollback did not restore FiredDirection and remote
streams were only sometimes restored. This resulted in not firing
ontrack if a track was rolled back and then added again on the same
transceiver.

Rollback also never performed OnTrack, which is incorrect because a
transceiver that goes from sendrecv to inactive will cause OnRemoveTrack
and if this is rolled back (so we become sendrecv again) then we need
OnTrack to fire.

This CL improves rollback's "memory", fires ontrack in Rollback() and
adds test coverage.

Needed to solve similar bugs in the Chromium layers as well:
https://chromium-review.googlesource.com/c/chromium/src/+/3613313

Bug: chromium:1320669
Change-Id: I655dd7d8a6b86080fe0e7c32c9e8c6434062ae91
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260330
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36734}

pc/peer_connection_jsep_unittest.cc

@@ -2233,6 +2233,60 @@ TEST_F(PeerConnectionJsepTest, RollbackRemoteDirectionChange) {
   EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u);
 }
 
+TEST_F(PeerConnectionJsepTest,
+       RollbackRestoresFiredDirectionAndOnTrackCanFireAgain) {
+  auto caller = CreatePeerConnection();
+  auto caller_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
+  auto callee = CreatePeerConnection();
+  callee->AddAudioTrack("a");
+  ASSERT_EQ(callee->pc()->GetTransceivers().size(), 1u);
+  auto callee_transceiver = callee->pc()->GetTransceivers()[0];
+  EXPECT_FALSE(callee_transceiver->fired_direction().has_value());
+  EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  EXPECT_TRUE(callee_transceiver->fired_direction().has_value());
+  EXPECT_EQ(callee->observer()->add_track_events_.size(), 1u);
+  // The existing transceiver becomes associated. Because it already exists,
+  // rolling it back does not remove the transceiver, so if ontrack fires again
+  // later it will be because the transceiver's internal states were restored
+  // rather than due to the creation of a new transceiver.
+  EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
+
+  // Rollback: the transceiver is no longer receiving.
+  EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback()));
+  EXPECT_FALSE(callee_transceiver->fired_direction().has_value());
+  EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
+
+  // Set the remote offer again: ontrack should fire on the same transceiver.
+  EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  EXPECT_TRUE(callee_transceiver->fired_direction().has_value());
+  EXPECT_EQ(callee->observer()->add_track_events_.size(), 2u);
+  EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
+}
+
+TEST_F(PeerConnectionJsepTest,
+       RollbackFromInactiveToReceivingMakesOnTrackFire) {
+  auto caller = CreatePeerConnection();
+  auto caller_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
+  auto callee = CreatePeerConnection();
+  // Perform full O/A so that transceiver is associated. Ontrack fires.
+  EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  EXPECT_EQ(callee->observer()->add_track_events_.size(), 1u);
+  EXPECT_EQ(callee->observer()->remove_track_events_.size(), 0u);
+  ASSERT_TRUE(
+      caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+
+  // Start negotiating to make the transceiver inactive. Onremovetrack fires.
+  caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive);
+  EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  EXPECT_EQ(callee->observer()->add_track_events_.size(), 1u);
+  EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u);
+
+  // Rollback the inactivation. Ontrack should fire again.
+  EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback()));
+  EXPECT_EQ(callee->observer()->add_track_events_.size(), 2u);
+  EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u);
+}
+
 TEST_F(PeerConnectionJsepTest, RollbackAfterMultipleSLD) {
   auto callee = CreatePeerConnection();
   callee->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);

pc/rtp_transceiver.cc

@@ -375,7 +375,8 @@ void RtpTransceiver::set_current_direction(RtpTransceiverDirection direction) {
   }
 }
 
-void RtpTransceiver::set_fired_direction(RtpTransceiverDirection direction) {
+void RtpTransceiver::set_fired_direction(
+    absl::optional<RtpTransceiverDirection> direction) {
   fired_direction_ = direction;
 }
 

pc/rtp_transceiver.h

@@ -201,8 +201,8 @@ class RtpTransceiver : public RtpTransceiverInterface,
 
   // 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).
+  // remote) after which the only valid reason to go back to null is rollback.
-  void set_fired_direction(RtpTransceiverDirection direction);
+  void set_fired_direction(absl::optional<RtpTransceiverDirection> direction);
 
   // According to JSEP rules for SetRemoteDescription, RtpTransceivers can be
   // reused only if they were added by AddTrack.

pc/sdp_offer_answer.cc

@@ -1952,6 +1952,13 @@ void SdpOfferAnswerHandler::ApplyRemoteDescriptionUpdateTransceiverState(
     const MediaContentDescription* media_desc = content->media_description();
     RtpTransceiverDirection local_direction =
         RtpTransceiverDirectionReversed(media_desc->direction());
+    // Remember the previous remote streams if this is a remote offer. This
+    // makes it possible to rollback modifications to the streams.
+    if (sdp_type == SdpType::kOffer) {
+      transceivers()
+          ->StableState(transceiver_ext)
+          ->SetRemoteStreamIds(transceiver->receiver()->stream_ids());
+    }
     // Roughly the same as steps 2.2.8.6 of section 4.4.1.6 "Set the
     // RTCSessionDescription: Set the associated remote streams given
     // transceiver.[[Receiver]], msids, addList, and removeList".
@@ -1962,9 +1969,6 @@ void SdpOfferAnswerHandler::ApplyRemoteDescriptionUpdateTransceiverState(
         // The remote description has signaled the stream IDs.
         stream_ids = media_desc->streams()[0].stream_ids();
       }
-      transceivers()
-          ->StableState(transceiver_ext)
-          ->SetRemoteStreamIdsIfUnset(transceiver->receiver()->stream_ids());
 
       RTC_LOG(LS_INFO) << "Processing the MSIDs for MID=" << content->name
                        << " (" << GetStreamIdsString(stream_ids) << ").";
@@ -1994,6 +1998,14 @@ void SdpOfferAnswerHandler::ApplyRemoteDescriptionUpdateTransceiverState(
                                   &removed_streams);
     }
     // 2.2.8.1.10: Set transceiver's [[FiredDirection]] slot to direction.
+    if (sdp_type == SdpType::kOffer) {
+      // Remember the previous fired direction if this is a remote offer. This
+      // makes it possible to rollback modifications to [[FiredDirection]],
+      // which is necessary for "ontrack" to fire in or after rollback.
+      transceivers()
+          ->StableState(transceiver_ext)
+          ->SetFiredDirection(transceiver->fired_direction());
+    }
     transceiver->set_fired_direction(local_direction);
     // 2.2.8.1.11: If description is of type "answer" or "pranswer", then run
     // the following steps:
@@ -2899,6 +2911,8 @@ RTCError SdpOfferAnswerHandler::Rollback(SdpType desc_type) {
   }
   RTC_DCHECK_RUN_ON(signaling_thread());
   RTC_DCHECK(IsUnifiedPlan());
+  std::vector<rtc::scoped_refptr<RtpTransceiverInterface>>
+      now_receiving_transceivers;
   std::vector<rtc::scoped_refptr<MediaStreamInterface>> all_added_streams;
   std::vector<rtc::scoped_refptr<MediaStreamInterface>> all_removed_streams;
   std::vector<rtc::scoped_refptr<RtpReceiverInterface>> removed_receivers;
@@ -2907,6 +2921,22 @@ RTCError SdpOfferAnswerHandler::Rollback(SdpType desc_type) {
     auto transceiver = transceivers_stable_state_pair.first;
     auto state = transceivers_stable_state_pair.second;
 
+    if (state.did_set_fired_direction()) {
+      // If this rollback triggers going from not receiving to receving again,
+      // we need to fire "ontrack".
+      bool previously_fired_direction_is_recv =
+          transceiver->fired_direction().has_value() &&
+          RtpTransceiverDirectionHasRecv(*transceiver->fired_direction());
+      bool currently_fired_direction_is_recv =
+          state.fired_direction().has_value() &&
+          RtpTransceiverDirectionHasRecv(state.fired_direction().value());
+      if (!previously_fired_direction_is_recv &&
+          currently_fired_direction_is_recv) {
+        now_receiving_transceivers.push_back(transceiver);
+      }
+      transceiver->internal()->set_fired_direction(state.fired_direction());
+    }
+
     if (state.remote_stream_ids()) {
       std::vector<rtc::scoped_refptr<MediaStreamInterface>> added_streams;
       std::vector<rtc::scoped_refptr<MediaStreamInterface>> removed_streams;
@@ -2923,6 +2953,10 @@ RTCError SdpOfferAnswerHandler::Rollback(SdpType desc_type) {
       }
     }
 
+    // Due to the above `continue` statement, the below code only runs if there
+    // is a change in mid association (has_m_section), if the transceiver was
+    // newly created (newly_created) or if remote streams were not set.
+
     RTC_DCHECK(transceiver->internal()->mid().has_value());
     transceiver->internal()->ClearChannel();
 
@@ -2957,6 +2991,11 @@ RTCError SdpOfferAnswerHandler::Rollback(SdpType desc_type) {
   ChangeSignalingState(PeerConnectionInterface::kStable);
 
   // Once all processing has finished, fire off callbacks.
+  for (const auto& transceiver : now_receiving_transceivers) {
+    pc_->Observer()->OnTrack(transceiver);
+    pc_->Observer()->OnAddTrack(transceiver->receiver(),
+                                transceiver->receiver()->streams());
+  }
   for (const auto& receiver : removed_receivers) {
     pc_->Observer()->OnRemoveTrack(receiver);
   }

pc/transceiver_list.cc

@@ -31,7 +31,7 @@ void TransceiverStableState::SetMSectionIfUnset(
   }
 }
 
-void TransceiverStableState::SetRemoteStreamIdsIfUnset(
+void TransceiverStableState::SetRemoteStreamIds(
     const std::vector<std::string>& ids) {
   if (!remote_stream_ids_.has_value()) {
     remote_stream_ids_ = ids;

pc/transceiver_list.h

@@ -43,9 +43,13 @@ class TransceiverStableState {
   void set_newly_created();
   void SetMSectionIfUnset(absl::optional<std::string> mid,
                           absl::optional<size_t> mline_index);
-  void SetRemoteStreamIdsIfUnset(const std::vector<std::string>& ids);
+  void SetRemoteStreamIds(const std::vector<std::string>& ids);
   void SetInitSendEncodings(
       const std::vector<RtpEncodingParameters>& encodings);
+  void SetFiredDirection(
+      absl::optional<RtpTransceiverDirection> fired_direction) {
+    fired_direction_ = fired_direction;
+  }
   absl::optional<std::string> mid() const { return mid_; }
   absl::optional<size_t> mline_index() const { return mline_index_; }
   absl::optional<std::vector<std::string>> remote_stream_ids() const {
@@ -57,6 +61,13 @@ class TransceiverStableState {
   }
   bool has_m_section() const { return has_m_section_; }
   bool newly_created() const { return newly_created_; }
+  bool did_set_fired_direction() const { return fired_direction_.has_value(); }
+  // Because fired_direction() is nullable, did_set_fired_direction() is used to
+  // distinguish beteen "no value" and "null value".
+  absl::optional<RtpTransceiverDirection> fired_direction() const {
+    RTC_DCHECK(did_set_fired_direction());
+    return fired_direction_.value();
+  }
 
  private:
   absl::optional<std::string> mid_;
@@ -71,6 +82,9 @@ class TransceiverStableState {
   // description to track potential need for removing transceiver during
   // rollback.
   bool newly_created_ = false;
+  // `fired_direction_` is nullable, so an optional of an optional is used to
+  // distinguish between null and not set (sorry if this hurts your eyes).
+  absl::optional<absl::optional<RtpTransceiverDirection>> fired_direction_;
 };
 
 // This class encapsulates the active list of transceivers on a