diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc index 0fa36c9549..851d9257e0 100644 --- a/pc/peer_connection_jsep_unittest.cc +++ b/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); diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index d7474185eb..2ada253327 100644 --- a/pc/rtp_transceiver.cc +++ b/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 direction) { fired_direction_ = direction; } diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index 5945d6a525..c0c987787c 100644 --- a/pc/rtp_transceiver.h +++ b/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). - void set_fired_direction(RtpTransceiverDirection direction); + // remote) after which the only valid reason to go back to null is rollback. + void set_fired_direction(absl::optional direction); // According to JSEP rules for SetRemoteDescription, RtpTransceivers can be // reused only if they were added by AddTrack. diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 5c1aa85f14..aa8e17a010 100644 --- a/pc/sdp_offer_answer.cc +++ b/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> + now_receiving_transceivers; std::vector> all_added_streams; std::vector> all_removed_streams; std::vector> 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> added_streams; std::vector> 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); } diff --git a/pc/transceiver_list.cc b/pc/transceiver_list.cc index 139c498634..250dfbc9e2 100644 --- a/pc/transceiver_list.cc +++ b/pc/transceiver_list.cc @@ -31,7 +31,7 @@ void TransceiverStableState::SetMSectionIfUnset( } } -void TransceiverStableState::SetRemoteStreamIdsIfUnset( +void TransceiverStableState::SetRemoteStreamIds( const std::vector& ids) { if (!remote_stream_ids_.has_value()) { remote_stream_ids_ = ids; diff --git a/pc/transceiver_list.h b/pc/transceiver_list.h index 568c9c7e7a..848ccc2c3b 100644 --- a/pc/transceiver_list.h +++ b/pc/transceiver_list.h @@ -43,9 +43,13 @@ class TransceiverStableState { void set_newly_created(); void SetMSectionIfUnset(absl::optional mid, absl::optional mline_index); - void SetRemoteStreamIdsIfUnset(const std::vector& ids); + void SetRemoteStreamIds(const std::vector& ids); void SetInitSendEncodings( const std::vector& encodings); + void SetFiredDirection( + absl::optional fired_direction) { + fired_direction_ = fired_direction; + } absl::optional mid() const { return mid_; } absl::optional mline_index() const { return mline_index_; } absl::optional> 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 fired_direction() const { + RTC_DCHECK(did_set_fired_direction()); + return fired_direction_.value(); + } private: absl::optional 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> fired_direction_; }; // This class encapsulates the active list of transceivers on a