From ef65ef18111311e3b523e9affb39e7d0502f0d96 Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Wed, 10 Jan 2018 17:15:20 -0800 Subject: [PATCH] Fire OnAddTrack when Unified Plan is enabled When Unified Plan semantics are set, PeerConnection will fire OnAddTrack according to the WebRTC spec. OnRemoveTrack will never be fired and will be deprecated in the future. Bug: webrtc:7600 Change-Id: Idfaada65b795b5fb9fe4844cff036d52ea90da17 Reviewed-on: https://webrtc-review.googlesource.com/38122 Commit-Queue: Steve Anton Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#21564} --- pc/peerconnection.cc | 26 ++++++--- pc/peerconnection.h | 5 ++ pc/peerconnection_jsep_unittest.cc | 79 ++++++++++++++++++++++++++ pc/peerconnectionwrapper.cc | 8 +-- pc/peerconnectionwrapper.h | 4 +- pc/rtpreceiver.cc | 82 ++++++++++++++++++++++++--- pc/rtpreceiver.h | 19 +++++-- pc/test/mockpeerconnectionobservers.h | 24 +++++++- 8 files changed, 219 insertions(+), 28 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index d6b441426e..0888acced2 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1980,6 +1980,7 @@ RTCError PeerConnection::ApplyRemoteDescription( } if (IsUnifiedPlan()) { + std::vector track_events; for (auto transceiver : transceivers_) { const ContentInfo* content = FindMediaSectionForTransceiver(transceiver, remote_description()); @@ -1997,15 +1998,27 @@ RTCError PeerConnection::ApplyRemoteDescription( (!transceiver->current_direction() || !RtpTransceiverDirectionHasRecv( *transceiver->current_direction()))) { - // TODO(bugs.webrtc.org/7600): Process the addition of a remote track. + const std::string& sync_label = media_desc->streams()[0].sync_label; + rtc::scoped_refptr stream = + remote_streams_->find(sync_label); + if (!stream) { + stream = MediaStreamProxy::Create(rtc::Thread::Current(), + MediaStream::Create(sync_label)); + remote_streams_->AddStream(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)); } // If direction is sendonly or inactive, and transceiver's current // direction is neither sendonly nor inactive, process the removal of a // remote track for the media description. if (!RtpTransceiverDirectionHasRecv(local_direction) && - (!transceiver->current_direction() || + (transceiver->current_direction() && RtpTransceiverDirectionHasRecv(*transceiver->current_direction()))) { - // TODO(bugs.webrtc.org/7600): Process the removal of a remote track. + transceiver->internal()->receiver_internal()->SetStreams({}); } if (type == SdpType::kPrAnswer || type == SdpType::kAnswer) { transceiver->internal()->set_current_direction(local_direction); @@ -2014,6 +2027,9 @@ RTCError PeerConnection::ApplyRemoteDescription( transceiver->Stop(); } } + for (auto event : track_events) { + observer_->OnAddTrack(event.receiver, event.streams); + } } const cricket::ContentInfo* audio_content = @@ -2760,8 +2776,6 @@ void PeerConnection::CreateAudioReceiver( new AudioRtpReceiver(worker_thread(), remote_sender_info.sender_id, streams, remote_sender_info.first_ssrc, voice_media_channel())); - stream->AddTrack( - static_cast(receiver->internal()->track().get())); GetAudioTransceiver()->internal()->AddReceiver(receiver); observer_->OnAddTrack(receiver, std::move(streams)); } @@ -2777,8 +2791,6 @@ void PeerConnection::CreateVideoReceiver( new VideoRtpReceiver(worker_thread(), remote_sender_info.sender_id, streams, remote_sender_info.first_ssrc, video_media_channel())); - stream->AddTrack( - static_cast(receiver->internal()->track().get())); GetVideoTransceiver()->internal()->AddReceiver(receiver); observer_->OnAddTrack(receiver, std::move(streams)); } diff --git a/pc/peerconnection.h b/pc/peerconnection.h index f2f833cc6b..d86f2975b3 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -314,6 +314,11 @@ class PeerConnection : public PeerConnectionInterface, 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_jsep_unittest.cc b/pc/peerconnection_jsep_unittest.cc index edab0820e4..0ba96e1a3b 100644 --- a/pc/peerconnection_jsep_unittest.cc +++ b/pc/peerconnection_jsep_unittest.cc @@ -39,6 +39,7 @@ using RTCConfiguration = PeerConnectionInterface::RTCConfiguration; using ::testing::Values; using ::testing::Combine; using ::testing::ElementsAre; +using ::testing::UnorderedElementsAre; class PeerConnectionFactoryForJsepTest : public PeerConnectionFactory { public: @@ -961,4 +962,82 @@ TEST_F(PeerConnectionJsepTest, EXPECT_EQ(receiver_id, receiver->id()); } +// Test that setting a remote offer with one track that has no streams fires off +// the correct OnAddTrack event. +TEST_F(PeerConnectionJsepTest, + SetRemoteOfferWithOneTrackNoStreamFiresOnAddTrack) { + const std::string kTrackLabel = "audio_track"; + + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + ASSERT_TRUE(caller->AddAudioTrack(kTrackLabel)); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); + EXPECT_EQ(kTrackLabel, + callee->observer()->add_track_events_[0].receiver->track()->id()); + // TODO(bugs.webrtc.org/7933): Also verify that no stream was added to the + // receiver. +} + +// Test that setting a remote offer with one track that has one stream fires off +// the correct OnAddTrack event. +TEST_F(PeerConnectionJsepTest, + SetRemoteOfferWithOneTrackOneStreamFiresOnAddTrack) { + const std::string kTrackLabel = "audio_track"; + const std::string kStreamLabel = "audio_stream"; + + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + ASSERT_TRUE(caller->AddAudioTrack(kTrackLabel, {kStreamLabel})); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + const auto& track_events = callee->observer()->add_track_events_; + ASSERT_EQ(1u, track_events.size()); + const auto& event = track_events[0]; + ASSERT_EQ(1u, event.streams.size()); + auto stream = event.streams[0]; + EXPECT_EQ(kStreamLabel, stream->label()); + EXPECT_THAT(track_events[0].snapshotted_stream_tracks.at(stream), + ElementsAre(event.receiver->track())); + EXPECT_EQ(event.receiver->streams(), track_events[0].streams); +} + +// Test that setting a remote offer with two tracks that share the same stream +// fires off two OnAddTrack events, both with the same stream that has both +// tracks present at the time of firing. This is to ensure that track events are +// not fired until SetRemoteDescription has finished processing all the media +// sections. +TEST_F(PeerConnectionJsepTest, + SetRemoteOfferWithTwoTracksSameStreamFiresOnAddTrack) { + const std::string kTrack1Label = "audio_track1"; + const std::string kTrack2Label = "audio_track2"; + const std::string kSharedStreamLabel = "stream"; + + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + ASSERT_TRUE(caller->AddAudioTrack(kTrack1Label, {kSharedStreamLabel})); + ASSERT_TRUE(caller->AddAudioTrack(kTrack2Label, {kSharedStreamLabel})); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + const auto& track_events = callee->observer()->add_track_events_; + ASSERT_EQ(2u, track_events.size()); + const auto& event1 = track_events[0]; + const auto& event2 = track_events[1]; + ASSERT_EQ(1u, event1.streams.size()); + auto stream = event1.streams[0]; + ASSERT_THAT(event2.streams, ElementsAre(stream)); + auto track1 = event1.receiver->track(); + auto track2 = event2.receiver->track(); + EXPECT_THAT(event1.snapshotted_stream_tracks.at(stream), + UnorderedElementsAre(track1, track2)); + EXPECT_THAT(event2.snapshotted_stream_tracks.at(stream), + UnorderedElementsAre(track1, track2)); +} + +// TODO(bugs.webrtc.org/7932): Also test multi-stream case. + } // namespace webrtc diff --git a/pc/peerconnectionwrapper.cc b/pc/peerconnectionwrapper.cc index 35aebcce9b..21d154289f 100644 --- a/pc/peerconnectionwrapper.cc +++ b/pc/peerconnectionwrapper.cc @@ -277,14 +277,14 @@ rtc::scoped_refptr PeerConnectionWrapper::AddTrack( rtc::scoped_refptr PeerConnectionWrapper::AddAudioTrack( const std::string& track_label, - std::vector streams) { - return pc()->AddTrack(CreateAudioTrack(track_label), std::move(streams)); + const std::vector& stream_labels) { + return AddTrack(CreateAudioTrack(track_label), stream_labels); } rtc::scoped_refptr PeerConnectionWrapper::AddVideoTrack( const std::string& track_label, - std::vector streams) { - return pc()->AddTrack(CreateVideoTrack(track_label), std::move(streams)); + const std::vector& stream_labels) { + return AddTrack(CreateVideoTrack(track_label), stream_labels); } rtc::scoped_refptr diff --git a/pc/peerconnectionwrapper.h b/pc/peerconnectionwrapper.h index c20c1f259f..b5aa16342b 100644 --- a/pc/peerconnectionwrapper.h +++ b/pc/peerconnectionwrapper.h @@ -140,13 +140,13 @@ class PeerConnectionWrapper { // stream track not bound to any source. rtc::scoped_refptr AddAudioTrack( const std::string& track_label, - std::vector streams = {}); + const std::vector& stream_labels = {}); // Calls the underlying PeerConnection's AddTrack method with a video media // stream track fed by a fake video capturer. rtc::scoped_refptr AddVideoTrack( const std::string& track_label, - std::vector streams = {}); + const std::vector& stream_labels = {}); // Calls the underlying PeerConnection's CreateDataChannel method with default // initialization parameters. diff --git a/pc/rtpreceiver.cc b/pc/rtpreceiver.cc index 33ad899010..98c501c22b 100644 --- a/pc/rtpreceiver.cc +++ b/pc/rtpreceiver.cc @@ -24,7 +24,7 @@ namespace webrtc { AudioRtpReceiver::AudioRtpReceiver( rtc::Thread* worker_thread, const std::string& receiver_id, - std::vector> streams, + const std::vector>& streams, uint32_t ssrc, cricket::VoiceMediaChannel* media_channel) : worker_thread_(worker_thread), @@ -35,12 +35,12 @@ AudioRtpReceiver::AudioRtpReceiver( AudioTrack::Create( receiver_id, RemoteAudioSource::Create(worker_thread, media_channel, ssrc)))), - streams_(std::move(streams)), cached_track_enabled_(track_->enabled()) { RTC_DCHECK(worker_thread_); RTC_DCHECK(track_->GetSource()->remote()); track_->RegisterObserver(this); track_->GetSource()->RegisterAudioObserver(this); + SetStreams(streams); SetMediaChannel(media_channel); Reconfigure(); } @@ -118,6 +118,39 @@ void AudioRtpReceiver::Stop() { stopped_ = true; } +void AudioRtpReceiver::SetStreams( + const std::vector>& streams) { + // Remove remote track from any streams that are going away. + for (auto existing_stream : streams_) { + bool removed = true; + for (auto stream : streams) { + if (existing_stream->label() == stream->label()) { + RTC_DCHECK_EQ(existing_stream.get(), stream.get()); + removed = false; + break; + } + } + if (removed) { + existing_stream->RemoveTrack(track_); + } + } + // Add remote track to any streams that are new. + for (auto stream : streams) { + bool added = true; + for (auto existing_stream : streams_) { + if (stream->label() == existing_stream->label()) { + RTC_DCHECK_EQ(stream.get(), existing_stream.get()); + added = false; + break; + } + } + if (added) { + stream->AddTrack(track_); + } + } + streams_ = streams; +} + std::vector AudioRtpReceiver::GetSources() const { return worker_thread_->Invoke>( RTC_FROM_HERE, [&] { return media_channel_->GetSources(ssrc_); }); @@ -157,12 +190,12 @@ void AudioRtpReceiver::NotifyFirstPacketReceived() { VideoRtpReceiver::VideoRtpReceiver( rtc::Thread* worker_thread, - const std::string& track_id, - std::vector> streams, + const std::string& receiver_id, + const std::vector>& streams, uint32_t ssrc, cricket::VideoMediaChannel* media_channel) : worker_thread_(worker_thread), - id_(track_id), + id_(receiver_id), ssrc_(ssrc), source_(new RefCountedObject(&broadcaster_, true /* remote */)), @@ -170,13 +203,13 @@ VideoRtpReceiver::VideoRtpReceiver( rtc::Thread::Current(), worker_thread, VideoTrack::Create( - track_id, + receiver_id, VideoTrackSourceProxy::Create(rtc::Thread::Current(), worker_thread, source_), - worker_thread))), - streams_(std::move(streams)) { + worker_thread))) { RTC_DCHECK(worker_thread_); + SetStreams(streams); source_->SetState(MediaSourceInterface::kLive); SetMediaChannel(media_channel); } @@ -229,6 +262,39 @@ void VideoRtpReceiver::Stop() { stopped_ = true; } +void VideoRtpReceiver::SetStreams( + const std::vector>& streams) { + // Remove remote track from any streams that are going away. + for (auto existing_stream : streams_) { + bool removed = true; + for (auto stream : streams) { + if (existing_stream->label() == stream->label()) { + RTC_DCHECK_EQ(existing_stream.get(), stream.get()); + removed = false; + break; + } + } + if (removed) { + existing_stream->RemoveTrack(track_); + } + } + // Add remote track to any streams that are new. + for (auto stream : streams) { + bool added = true; + for (auto existing_stream : streams_) { + if (stream->label() == existing_stream->label()) { + RTC_DCHECK_EQ(stream.get(), existing_stream.get()); + added = false; + break; + } + } + if (added) { + stream->AddTrack(track_); + } + } + streams_ = streams; +} + void VideoRtpReceiver::SetObserver(RtpReceiverObserverInterface* observer) { observer_ = observer; // Deliver any notifications the observer may have missed by being set late. diff --git a/pc/rtpreceiver.h b/pc/rtpreceiver.h index 0ce637337e..e3848a7c5b 100644 --- a/pc/rtpreceiver.h +++ b/pc/rtpreceiver.h @@ -33,6 +33,7 @@ namespace webrtc { class RtpReceiverInternal : public RtpReceiverInterface { public: virtual void Stop() = 0; + // This SSRC is used as an identifier for the receiver between the API layer // and the WebRtcVideoEngine, WebRtcVoiceEngine layer. virtual uint32_t ssrc() const = 0; @@ -40,6 +41,12 @@ class RtpReceiverInternal : public RtpReceiverInterface { // Call this to notify the RtpReceiver when the first packet has been received // on the corresponding channel. virtual void NotifyFirstPacketReceived() = 0; + + // Set the associated remote media streams for this receiver. The remote track + // will be removed from any streams that are no longer present and added to + // any new streams. + virtual void SetStreams( + const std::vector>& streams) = 0; }; class AudioRtpReceiver : public ObserverInterface, @@ -53,7 +60,7 @@ class AudioRtpReceiver : public ObserverInterface, AudioRtpReceiver( rtc::Thread* worker_thread, const std::string& receiver_id, - std::vector> streams, + const std::vector>& streams, uint32_t ssrc, cricket::VoiceMediaChannel* media_channel); virtual ~AudioRtpReceiver(); @@ -90,6 +97,8 @@ class AudioRtpReceiver : public ObserverInterface, void Stop() override; uint32_t ssrc() const override { return ssrc_; } void NotifyFirstPacketReceived() override; + void SetStreams(const std::vector>& + streams) override; void SetObserver(RtpReceiverObserverInterface* observer) override; @@ -122,8 +131,8 @@ class VideoRtpReceiver : public rtc::RefCountedObject { // sees. VideoRtpReceiver( rtc::Thread* worker_thread, - const std::string& track_id, - std::vector> streams, + const std::string& receiver_id, + const std::vector>& streams, uint32_t ssrc, cricket::VideoMediaChannel* media_channel); @@ -155,6 +164,8 @@ class VideoRtpReceiver : public rtc::RefCountedObject { void Stop() override; uint32_t ssrc() const override { return ssrc_; } void NotifyFirstPacketReceived() override; + void SetStreams(const std::vector>& + streams) override; void SetObserver(RtpReceiverObserverInterface* observer) override; @@ -166,7 +177,7 @@ class VideoRtpReceiver : public rtc::RefCountedObject { bool SetSink(rtc::VideoSinkInterface* sink); rtc::Thread* const worker_thread_; - std::string id_; + const std::string id_; uint32_t ssrc_; cricket::VideoMediaChannel* media_channel_ = nullptr; // |broadcaster_| is needed since the decoder can only handle one sink. diff --git a/pc/test/mockpeerconnectionobservers.h b/pc/test/mockpeerconnectionobservers.h index 5eb29f4a55..86f22368d2 100644 --- a/pc/test/mockpeerconnectionobservers.h +++ b/pc/test/mockpeerconnectionobservers.h @@ -14,6 +14,7 @@ #ifndef PC_TEST_MOCKPEERCONNECTIONOBSERVERS_H_ #define PC_TEST_MOCKPEERCONNECTIONOBSERVERS_H_ +#include #include #include #include @@ -31,12 +32,29 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { public: struct AddTrackEvent { explicit AddTrackEvent( - rtc::scoped_refptr receiver, - std::vector> streams) - : receiver(std::move(receiver)), streams(std::move(streams)) {} + rtc::scoped_refptr event_receiver, + std::vector> event_streams) + : receiver(std::move(event_receiver)), + streams(std::move(event_streams)) { + for (auto stream : streams) { + std::vector> tracks; + for (auto audio_track : stream->GetAudioTracks()) { + tracks.push_back(audio_track); + } + for (auto video_track : stream->GetVideoTracks()) { + tracks.push_back(video_track); + } + snapshotted_stream_tracks[stream] = tracks; + } + } rtc::scoped_refptr receiver; std::vector> streams; + // This map records the tracks present in each stream at the time the + // OnAddTrack callback was issued. + std::map, + std::vector>> + snapshotted_stream_tracks; }; MockPeerConnectionObserver() : remote_streams_(StreamCollection::Create()) {}