diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index 5786859094..5784d9500a 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -1050,10 +1050,25 @@ class PeerConnectionObserver { // This is called when a receiver and its track is created. // TODO(zhihuang): Make this pure virtual when all subclasses implement it. + // Note: This is called with both Plan B and Unified Plan semantics. Unified + // Plan users should prefer OnTrack, OnAddTrack is only called as backwards + // compatibility (and is called in the exact same situations as OnTrack). virtual void OnAddTrack( rtc::scoped_refptr receiver, const std::vector>& streams) {} + // This is called when signaling indicates a transceiver will be receiving + // media from the remote endpoint. This is fired during a call to + // SetRemoteDescription. The receiving track can be accessed by: + // |transceiver->receiver()->track()| and its associated streams by + // |transceiver->receiver()->streams()|. + // Note: This will only be called if Unified Plan semantics are specified. + // This behavior is specified in section 2.2.8.2.5 of the "Set the + // RTCSessionDescription" algorithm: + // https://w3c.github.io/webrtc-pc/#set-description + virtual void OnTrack( + rtc::scoped_refptr transceiver) {} + // TODO(hbos,deadbeef): Add |OnAssociatedStreamsUpdated| with |receiver| and // |streams| as arguments. This should be called when an existing receiver its // associated streams updated. https://crbug.com/webrtc/8315 @@ -1065,7 +1080,7 @@ class PeerConnectionObserver { // behavior that occurs when processing the removal of a remote track, and is // called when the receiver is removed and the track is muted. When Unified // Plan SDP is supported, transceivers can change direction (and receivers - // stopped) but receivers are never removed. + // stopped) but receivers are never removed, so this is never called. // https://w3c.github.io/webrtc-pc/#process-remote-track-removal // TODO(hbos,deadbeef): When Unified Plan SDP is supported and receivers are // no longer removed, deprecate and remove this callback. diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 39822678fc..f04b6e5667 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -2155,7 +2155,8 @@ RTCError PeerConnection::ApplyRemoteDescription( } if (IsUnifiedPlan()) { - std::vector track_events; + std::vector> + receiving_transceivers; std::vector> added_streams; for (auto transceiver : transceivers_) { const ContentInfo* content = @@ -2185,10 +2186,7 @@ RTCError PeerConnection::ApplyRemoteDescription( added_streams.push_back(stream); } transceiver->internal()->receiver_internal()->SetStreams({stream}); - TrackEvent track_event; - track_event.receiver = transceiver->receiver(); - track_event.streams = transceiver->receiver()->streams(); - track_events.push_back(std::move(track_event)); + receiving_transceivers.push_back(transceiver); } // If direction is sendonly or inactive, and transceiver's current // direction is neither sendonly nor inactive, process the removal of a @@ -2210,8 +2208,10 @@ RTCError PeerConnection::ApplyRemoteDescription( } } // Once all processing has finished, fire off callbacks. - for (auto event : track_events) { - observer_->OnAddTrack(event.receiver, event.streams); + for (auto transceiver : receiving_transceivers) { + observer_->OnTrack(transceiver); + observer_->OnAddTrack(transceiver->receiver(), + transceiver->receiver()->streams()); } for (auto stream : added_streams) { observer_->OnAddStream(stream); diff --git a/pc/peerconnection.h b/pc/peerconnection.h index e6d7b7ee9d..3eccb96b96 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -292,11 +292,6 @@ class PeerConnection : public PeerConnectionInternal, uint32_t first_ssrc; }; - struct TrackEvent { - rtc::scoped_refptr receiver; - std::vector> streams; - }; - // Implements MessageHandler. void OnMessage(rtc::Message* msg) override; diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc index 2f20261652..09d2dbe216 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -219,6 +219,106 @@ TEST_F(PeerConnectionRtpCallbacksTest, callee->observer()->remove_track_events_); } +// Tests that setting a remote description with sending transceivers will fire +// the OnTrack callback for each transceiver and setting a remote description +// with receive only transceivers will not call OnTrack. +TEST_F(PeerConnectionRtpCallbacksTest, UnifiedPlanAddTransceiverCallsOnTrack) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + auto callee = CreatePeerConnectionWithUnifiedPlan(); + + auto audio_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + auto video_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + + ASSERT_EQ(0u, caller->observer()->on_track_transceivers_.size()); + ASSERT_EQ(2u, callee->observer()->on_track_transceivers_.size()); + EXPECT_EQ(audio_transceiver->mid(), + callee->pc()->GetTransceivers()[0]->mid()); + EXPECT_EQ(video_transceiver->mid(), + callee->pc()->GetTransceivers()[1]->mid()); +} + +// Test that doing additional offer/answer exchanges with no changes to tracks +// will cause no additional OnTrack calls after the tracks have been negotiated. +TEST_F(PeerConnectionRtpCallbacksTest, UnifiedPlanReofferDoesNotCallOnTrack) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + auto callee = CreatePeerConnectionWithUnifiedPlan(); + + caller->AddAudioTrack("audio"); + callee->AddAudioTrack("audio"); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + EXPECT_EQ(1u, caller->observer()->on_track_transceivers_.size()); + EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); + + // If caller reoffers with no changes expect no additional OnTrack calls. + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + EXPECT_EQ(1u, caller->observer()->on_track_transceivers_.size()); + EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); + + // Also if callee reoffers with no changes expect no additional OnTrack calls. + ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get())); + EXPECT_EQ(1u, caller->observer()->on_track_transceivers_.size()); + EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); +} + +// Test that OnTrack is called when the transceiver direction changes to send +// the track. +TEST_F(PeerConnectionRtpCallbacksTest, UnifiedPlanSetDirectionCallsOnTrack) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + auto callee = CreatePeerConnectionWithUnifiedPlan(); + + auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + transceiver->SetDirection(RtpTransceiverDirection::kInactive); + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); + EXPECT_EQ(0u, callee->observer()->on_track_transceivers_.size()); + + transceiver->SetDirection(RtpTransceiverDirection::kSendOnly); + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); + EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); + + // If the direction changes but it is still receiving on the remote side, then + // OnTrack should not be fired again. + transceiver->SetDirection(RtpTransceiverDirection::kSendRecv); + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); + EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); +} + +// Test that OnTrack is called twice when a sendrecv call is started, the callee +// changes the direction to inactive, then changes it back to sendrecv. +TEST_F(PeerConnectionRtpCallbacksTest, + UnifiedPlanSetDirectionHoldCallsOnTrackTwice) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + auto callee = CreatePeerConnectionWithUnifiedPlan(); + + auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); + EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); + + // Put the call on hold by no longer receiving the track. + callee->pc()->GetTransceivers()[0]->SetDirection( + RtpTransceiverDirection::kInactive); + + ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get())); + EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); + EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); + + // Resume the call by changing the direction to recvonly. This should call + // OnTrack again on the callee side. + callee->pc()->GetTransceivers()[0]->SetDirection( + RtpTransceiverDirection::kRecvOnly); + + ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get())); + EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); + EXPECT_EQ(2u, callee->observer()->on_track_transceivers_.size()); +} + // These tests examine the state of the peer connection as a result of // performing SetRemoteDescription(). class PeerConnectionRtpObserverTest : public PeerConnectionRtpTest {}; diff --git a/pc/test/mockpeerconnectionobservers.h b/pc/test/mockpeerconnectionobservers.h index a7e488439e..10979fd31a 100644 --- a/pc/test/mockpeerconnectionobservers.h +++ b/pc/test/mockpeerconnectionobservers.h @@ -132,6 +132,11 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { add_track_events_.push_back(AddTrackEvent(receiver, streams)); } + void OnTrack( + rtc::scoped_refptr transceiver) override { + on_track_transceivers_.push_back(transceiver); + } + void OnRemoveTrack( rtc::scoped_refptr receiver) override { remove_track_events_.push_back(receiver); @@ -209,6 +214,8 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { std::string last_added_track_label_; std::vector add_track_events_; std::vector> remove_track_events_; + std::vector> + on_track_transceivers_; int num_candidates_removed_ = 0; private: