diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 3b2f71c64a..211c3d6c1c 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1157,9 +1157,11 @@ PeerConnection::AddTrackUnifiedPlan( auto transceiver = FindFirstTransceiverForAddedTrack(track); if (transceiver) { if (transceiver->direction() == RtpTransceiverDirection::kRecvOnly) { - transceiver->SetDirection(RtpTransceiverDirection::kSendRecv); + transceiver->internal()->set_direction( + RtpTransceiverDirection::kSendRecv); } else if (transceiver->direction() == RtpTransceiverDirection::kInactive) { - transceiver->SetDirection(RtpTransceiverDirection::kSendOnly); + transceiver->internal()->set_direction( + RtpTransceiverDirection::kSendOnly); } transceiver->sender()->SetTrack(track); transceiver->internal()->sender_internal()->set_stream_ids(stream_labels); @@ -1172,7 +1174,7 @@ PeerConnection::AddTrackUnifiedPlan( auto receiver = CreateReceiver(media_type, rtc::CreateRandomUuid()); transceiver = CreateAndAddTransceiver(sender, receiver); transceiver->internal()->set_created_by_addtrack(true); - transceiver->SetDirection(RtpTransceiverDirection::kSendRecv); + transceiver->internal()->set_direction(RtpTransceiverDirection::kSendRecv); } return transceiver->sender(); } @@ -1214,9 +1216,11 @@ RTCError PeerConnection::RemoveTrackInternal( } sender->SetTrack(nullptr); if (transceiver->direction() == RtpTransceiverDirection::kSendRecv) { - transceiver->internal()->SetDirection(RtpTransceiverDirection::kRecvOnly); + transceiver->internal()->set_direction( + RtpTransceiverDirection::kRecvOnly); } else if (transceiver->direction() == RtpTransceiverDirection::kSendOnly) { - transceiver->internal()->SetDirection(RtpTransceiverDirection::kInactive); + transceiver->internal()->set_direction( + RtpTransceiverDirection::kInactive); } } else { bool removed; @@ -1390,9 +1394,17 @@ PeerConnection::CreateAndAddTransceiver( auto transceiver = RtpTransceiverProxyWithInternal::Create( signaling_thread(), new RtpTransceiver(sender, receiver)); transceivers_.push_back(transceiver); + transceiver->internal()->SignalNegotiationNeeded.connect( + this, &PeerConnection::OnNegotiationNeeded); return transceiver; } +void PeerConnection::OnNegotiationNeeded() { + RTC_DCHECK_RUN_ON(signaling_thread()); + RTC_DCHECK(!IsClosed()); + observer_->OnRenegotiationNeeded(); +} + rtc::scoped_refptr PeerConnection::CreateDtmfSender( AudioTrackInterface* track) { TRACE_EVENT0("webrtc", "PeerConnection::CreateDtmfSender"); diff --git a/pc/peerconnection.h b/pc/peerconnection.h index e6ced417c2..7171deec9b 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -449,6 +449,8 @@ class PeerConnection : public PeerConnectionInternal, transceiver, const SessionDescriptionInterface* sdesc) const; + void OnNegotiationNeeded(); + bool IsClosed() const { return signaling_state_ == PeerConnectionInterface::kClosed; } diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc index 09d2dbe216..351905dcac 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -976,6 +976,46 @@ TEST_F(PeerConnectionRtpUnifiedPlanTest, EXPECT_FALSE(caller->observer()->negotiation_needed()); } +// Test that OnRenegotiationNeeded is fired if SetDirection is called on an +// active RtpTransceiver with a new direction. +TEST_F(PeerConnectionRtpUnifiedPlanTest, + RenegotiationNeededAfterTransceiverSetDirection) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + + auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + + caller->observer()->clear_negotiation_needed(); + transceiver->SetDirection(RtpTransceiverDirection::kInactive); + EXPECT_TRUE(caller->observer()->negotiation_needed()); +} + +// Test that OnRenegotiationNeeded is not fired if SetDirection is called on an +// active RtpTransceiver with current direction. +TEST_F(PeerConnectionRtpUnifiedPlanTest, + NoRenegotiationNeededAfterTransceiverSetSameDirection) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + + auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + + caller->observer()->clear_negotiation_needed(); + transceiver->SetDirection(transceiver->direction()); + EXPECT_FALSE(caller->observer()->negotiation_needed()); +} + +// Test that OnRenegotiationNeeded is not fired if SetDirection is called on a +// stopped RtpTransceiver. +TEST_F(PeerConnectionRtpUnifiedPlanTest, + NoRenegotiationNeededAfterSetDirectionOnStoppedTransceiver) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + + auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + transceiver->Stop(); + + caller->observer()->clear_negotiation_needed(); + transceiver->SetDirection(RtpTransceiverDirection::kInactive); + EXPECT_FALSE(caller->observer()->negotiation_needed()); +} + // Test MSID signaling between Unified Plan and Plan B endpoints. There are two // options for this kind of signaling: media section based (a=msid) and ssrc // based (a=ssrc MSID). While JSEP only specifies media section MSID signaling, diff --git a/pc/rtptransceiver.cc b/pc/rtptransceiver.cc index d4a67883c3..7ef6ab2950 100644 --- a/pc/rtptransceiver.cc +++ b/pc/rtptransceiver.cc @@ -190,8 +190,14 @@ RtpTransceiverDirection RtpTransceiver::direction() const { } void RtpTransceiver::SetDirection(RtpTransceiverDirection new_direction) { - // TODO(steveanton): This should fire OnNegotiationNeeded. - set_direction(new_direction); + if (stopped()) { + return; + } + if (new_direction == direction_) { + return; + } + direction_ = new_direction; + SignalNegotiationNeeded(); } rtc::Optional RtpTransceiver::current_direction() diff --git a/pc/rtptransceiver.h b/pc/rtptransceiver.h index e44b4aeaef..3cefda2e1f 100644 --- a/pc/rtptransceiver.h +++ b/pc/rtptransceiver.h @@ -154,6 +154,10 @@ class RtpTransceiver final return has_ever_been_used_to_send_; } + // Fired when the RtpTransceiver state changes such that negotiation is now + // needed (e.g., in response to a direction change). + sigslot::signal0<> SignalNegotiationNeeded; + // RtpTransceiverInterface implementation. cricket::MediaType media_type() const override; rtc::Optional mid() const override;