diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index 7291a8641c..1039cb5a90 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -868,13 +868,31 @@ class PeerConnectionObserver { // Called when the ICE connection receiving status changes. virtual void OnIceConnectionReceivingChange(bool receiving) {} - // Called when a track is added to streams. - // TODO(zhihuang) Make this a pure virtual method when all its subclasses - // implement it. + // This is called when a receiver and its track is created. + // TODO(zhihuang): Make this pure virtual when all subclasses implement it. virtual void OnAddTrack( rtc::scoped_refptr receiver, const std::vector>& streams) {} + // 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 + // This may be blocked on supporting multiple streams per sender or else + // this may count as the removal and addition of a track? + // https://crbug.com/webrtc/7932 + + // Called when a receiver is completely removed. This is current (Plan B SDP) + // 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. + // 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. + // TODO(hbos,deadbeef): Make pure virtual when all subclasses implement it. + virtual void OnRemoveTrack( + rtc::scoped_refptr receiver) {} + protected: // Dtor protected as objects shouldn't be deleted via this interface. ~PeerConnectionObserver() {} diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 6e95bf8a8f..950a6ee978 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -393,6 +393,7 @@ if (rtc_include_tests) { "mediastream_unittest.cc", "peerconnection_crypto_unittest.cc", "peerconnection_integrationtest.cc", + "peerconnection_rtp_unittest.cc", "peerconnectionendtoend_unittest.cc", "peerconnectionfactory_unittest.cc", "peerconnectioninterface_unittest.cc", @@ -467,6 +468,7 @@ if (rtc_include_tests) { "../p2p:p2p_test_utils", "../p2p:rtc_p2p", "../pc:rtc_pc", + "../rtc_base:rtc_base", "../rtc_base:rtc_base_approved", "../rtc_base:rtc_base_tests_main", "../rtc_base:rtc_base_tests_utils", diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index bc95895a8e..e8e428dd4f 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1462,15 +1462,18 @@ void PeerConnection::CreateVideoReceiver(MediaStreamInterface* stream, // TODO(deadbeef): Keep RtpReceivers around even if track goes away in remote // description. -void PeerConnection::DestroyReceiver(const std::string& track_id) { +rtc::scoped_refptr PeerConnection::RemoveAndStopReceiver( + const std::string& track_id) { auto it = FindReceiverForTrack(track_id); if (it == receivers_.end()) { LOG(LS_WARNING) << "RtpReceiver for track with id " << track_id << " doesn't exist."; - } else { - (*it)->internal()->Stop(); - receivers_.erase(it); + return nullptr; } + (*it)->internal()->Stop(); + rtc::scoped_refptr receiver = *it; + receivers_.erase(it); + return receiver; } void PeerConnection::AddAudioTrack(AudioTrackInterface* track, @@ -2012,10 +2015,11 @@ void PeerConnection::OnRemoteTrackRemoved(const std::string& stream_label, cricket::MediaType media_type) { MediaStreamInterface* stream = remote_streams_->find(stream_label); + rtc::scoped_refptr receiver; if (media_type == cricket::MEDIA_TYPE_AUDIO) { // When the MediaEngine audio channel is destroyed, the RemoteAudioSource // will be notified which will end the AudioRtpReceiver::track(). - DestroyReceiver(track_id); + receiver = RemoveAndStopReceiver(track_id); rtc::scoped_refptr audio_track = stream->FindAudioTrack(track_id); if (audio_track) { @@ -2024,7 +2028,7 @@ void PeerConnection::OnRemoteTrackRemoved(const std::string& stream_label, } else if (media_type == cricket::MEDIA_TYPE_VIDEO) { // Stopping or destroying a VideoRtpReceiver will end the // VideoRtpReceiver::track(). - DestroyReceiver(track_id); + receiver = RemoveAndStopReceiver(track_id); rtc::scoped_refptr video_track = stream->FindVideoTrack(track_id); if (video_track) { @@ -2035,6 +2039,9 @@ void PeerConnection::OnRemoteTrackRemoved(const std::string& stream_label, } else { RTC_NOTREACHED() << "Invalid media type"; } + if (receiver) { + observer_->OnRemoveTrack(receiver); + } } void PeerConnection::UpdateEndedRemoteMediaStreams() { diff --git a/pc/peerconnection.h b/pc/peerconnection.h index fa06c3d78d..9d49f934b5 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -246,7 +246,8 @@ class PeerConnection : public PeerConnectionInterface, void CreateVideoReceiver(MediaStreamInterface* stream, const std::string& track_id, uint32_t ssrc); - void DestroyReceiver(const std::string& track_id); + rtc::scoped_refptr RemoveAndStopReceiver( + const std::string& track_id); // May be called either by AddStream/RemoveStream, or when a track is // added/removed from a stream previously added via AddStream. diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc new file mode 100644 index 0000000000..38ba5fbd67 --- /dev/null +++ b/pc/peerconnection_rtp_unittest.cc @@ -0,0 +1,165 @@ +/* + * Copyright 2017 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include +#include + +#include "api/jsep.h" +#include "api/mediastreaminterface.h" +#include "api/peerconnectioninterface.h" +#include "pc/mediastream.h" +#include "pc/mediastreamtrack.h" +#include "pc/peerconnectionwrapper.h" +#include "pc/test/fakeaudiocapturemodule.h" +#include "pc/test/mockpeerconnectionobservers.h" +#include "rtc_base/checks.h" +#include "rtc_base/gunit.h" +#include "rtc_base/ptr_util.h" +#include "rtc_base/refcountedobject.h" +#include "rtc_base/scoped_ref_ptr.h" +#include "rtc_base/thread.h" + +// This file contains tests for RTP Media API-related behavior of +// |webrtc::PeerConnection|, see https://w3c.github.io/webrtc-pc/#rtp-media-api. + +namespace { + +class PeerConnectionRtpTest : public testing::Test { + public: + PeerConnectionRtpTest() + : + pc_factory_(webrtc::CreatePeerConnectionFactory( + rtc::Thread::Current(), + rtc::Thread::Current(), + rtc::Thread::Current(), + FakeAudioCaptureModule::Create(), + nullptr, + nullptr)) {} + + std::unique_ptr CreatePeerConnection() { + webrtc::PeerConnectionInterface::RTCConfiguration config; + auto observer = rtc::MakeUnique(); + auto pc = pc_factory_->CreatePeerConnection(config, nullptr, nullptr, + observer.get()); + return std::unique_ptr( + new webrtc::PeerConnectionWrapper(pc_factory_, pc, + std::move(observer))); + } + + protected: + rtc::scoped_refptr pc_factory_; +}; + +TEST_F(PeerConnectionRtpTest, AddTrackWithoutStreamFiresOnAddTrack) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track( + pc_factory_->CreateAudioTrack("audio_track", nullptr)); + EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {})); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + // TODO(deadbeef): When no stream is handled correctly we would expect + // |add_track_events_[0].streams| to be empty. https://crbug.com/webrtc/7933 + ASSERT_EQ(1u, callee->observer()->add_track_events_[0].streams.size()); + EXPECT_TRUE( + callee->observer()->add_track_events_[0].streams[0]->FindAudioTrack( + "audio_track")); +} + +TEST_F(PeerConnectionRtpTest, AddTrackWithStreamFiresOnAddTrack) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track( + pc_factory_->CreateAudioTrack("audio_track", nullptr)); + auto stream = webrtc::MediaStream::Create("audio_stream"); + EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {stream.get()})); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + ASSERT_EQ(1u, callee->observer()->add_track_events_[0].streams.size()); + EXPECT_EQ("audio_stream", + callee->observer()->add_track_events_[0].streams[0]->label()); + EXPECT_TRUE( + callee->observer()->add_track_events_[0].streams[0]->FindAudioTrack( + "audio_track")); +} + +TEST_F(PeerConnectionRtpTest, RemoveTrackWithoutStreamFiresOnRemoveTrack) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track( + pc_factory_->CreateAudioTrack("audio_track", nullptr)); + auto sender = caller->pc()->AddTrack(audio_track.get(), {}); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + EXPECT_TRUE(caller->pc()->RemoveTrack(sender)); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), + callee->observer()->remove_track_events_); +} + +TEST_F(PeerConnectionRtpTest, RemoveTrackWithStreamFiresOnRemoveTrack) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track( + pc_factory_->CreateAudioTrack("audio_track", nullptr)); + auto stream = webrtc::MediaStream::Create("audio_stream"); + auto sender = caller->pc()->AddTrack(audio_track.get(), {stream.get()}); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + EXPECT_TRUE(caller->pc()->RemoveTrack(sender)); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + ASSERT_EQ(1u, callee->observer()->add_track_events_.size()); + EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), + callee->observer()->remove_track_events_); +} + +TEST_F(PeerConnectionRtpTest, RemoveTrackWithSharedStreamFiresOnRemoveTrack) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + rtc::scoped_refptr audio_track1( + pc_factory_->CreateAudioTrack("audio_track1", nullptr)); + rtc::scoped_refptr audio_track2( + pc_factory_->CreateAudioTrack("audio_track2", nullptr)); + auto stream = webrtc::MediaStream::Create("shared_audio_stream"); + std::vector streams{stream.get()}; + auto sender1 = caller->pc()->AddTrack(audio_track1.get(), streams); + auto sender2 = caller->pc()->AddTrack(audio_track2.get(), streams); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + ASSERT_EQ(2u, callee->observer()->add_track_events_.size()); + + // Remove "audio_track1". + EXPECT_TRUE(caller->pc()->RemoveTrack(sender1)); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_EQ(2u, callee->observer()->add_track_events_.size()); + EXPECT_EQ( + std::vector>{ + callee->observer()->add_track_events_[0].receiver}, + callee->observer()->remove_track_events_); + + // Remove "audio_track2". + EXPECT_TRUE(caller->pc()->RemoveTrack(sender2)); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_EQ(2u, callee->observer()->add_track_events_.size()); + EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), + callee->observer()->remove_track_events_); +} + +} // namespace diff --git a/pc/test/mockpeerconnectionobservers.h b/pc/test/mockpeerconnectionobservers.h index 64f84b6b8e..119c32b0f5 100644 --- a/pc/test/mockpeerconnectionobservers.h +++ b/pc/test/mockpeerconnectionobservers.h @@ -25,6 +25,16 @@ namespace webrtc { 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 receiver; + std::vector> streams; + }; + MockPeerConnectionObserver() : remote_streams_(StreamCollection::Create()) {} virtual ~MockPeerConnectionObserver() {} void SetPeerConnectionInterface(PeerConnectionInterface* pc) { @@ -92,13 +102,26 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { callback_triggered_ = true; } - void OnAddTrack( - rtc::scoped_refptr receiver, - const std::vector>& - streams) override { + void OnAddTrack(rtc::scoped_refptr receiver, + const std::vector>& + streams) override { RTC_DCHECK(receiver); num_added_tracks_++; last_added_track_label_ = receiver->id(); + add_track_events_.push_back(AddTrackEvent(receiver, streams)); + } + + void OnRemoveTrack( + rtc::scoped_refptr receiver) override { + remove_track_events_.push_back(receiver); + } + + std::vector> GetAddTrackReceivers() { + std::vector> receivers; + for (const AddTrackEvent& event : add_track_events_) { + receivers.push_back(event.receiver); + } + return receivers; } // Returns the label of the last added stream. @@ -124,6 +147,8 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { bool callback_triggered_ = false; int num_added_tracks_ = 0; std::string last_added_track_label_; + std::vector add_track_events_; + std::vector> remove_track_events_; private: rtc::scoped_refptr last_added_stream_; diff --git a/tools_webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt b/tools_webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt index 497f1f0b8f..8f10317aae 100644 --- a/tools_webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt +++ b/tools_webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt @@ -1,8 +1,9 @@ # Tests that are failing when run under memcheck. # https://code.google.com/p/webrtc/issues/detail?id=4387 DtmfSenderTest.* -PeerConnectionIntegrationTest.* PeerConnectionEndToEndTest.* -PeerConnectionInterfaceTest.* PeerConnectionCryptoUnitTest.* +PeerConnectionIntegrationTest.* +PeerConnectionInterfaceTest.* +PeerConnectionRtpTest.* RTCStatsIntegrationTest.*