From 933d8b07ea8ec5d1b2a04de277a656a6713e9d4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Tue, 10 Oct 2017 10:05:16 -0700 Subject: [PATCH] Reland "Added PeerConnectionObserver::OnRemoveTrack." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 6c0c55c31817ecfa32409424495eb68b31828c40. Reason for revert: Fixed the flake. Original change's description: > Revert "Added PeerConnectionObserver::OnRemoveTrack." > > This reverts commit ba97ba7af917d4152f5f3363aba1c1561c6673dc. > > Reason for revert: The new tests have caused several test failures on the test bots; the method FakeAudioMediaStreamTrack:GetSignalLevel, which is not supposed to be called is sometimes called anyway. > > Original change's description: > > Added PeerConnectionObserver::OnRemoveTrack. > > > > This corresponds to processing the removal of a remote track step of > > the spec, with processing the addition of a remote track already > > covered by OnAddTrack. > > https://w3c.github.io/webrtc-pc/#processing-remote-mediastreamtracks > > > > Bug: webrtc:8260, webrtc:8315 > > Change-Id: Ica8be92369733eb3cf1397fb60385d45a9b58700 > > Reviewed-on: https://webrtc-review.googlesource.com/4722 > > Commit-Queue: Henrik Boström > > Reviewed-by: Taylor Brandstetter > > Reviewed-by: Steve Anton > > Cr-Commit-Position: refs/heads/master@{#20214} > > TBR=steveanton@webrtc.org,deadbeef@webrtc.org,hbos@webrtc.org > > Change-Id: Id2d9533e27227254769b4280a8ff10a47313e714 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: webrtc:8260, webrtc:8315 > Reviewed-on: https://webrtc-review.googlesource.com/7940 > Reviewed-by: Alex Loiko > Commit-Queue: Alex Loiko > Cr-Commit-Position: refs/heads/master@{#20218} TBR=steveanton@webrtc.org,deadbeef@webrtc.org,aleloi@webrtc.org,hbos@webrtc.org Change-Id: Iab7500bebf98535754b102874259de43831fff6b No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:8260, webrtc:8315 Reviewed-on: https://webrtc-review.googlesource.com/8180 Commit-Queue: Henrik Boström Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#20227} --- api/peerconnectioninterface.h | 24 ++- pc/BUILD.gn | 2 + pc/peerconnection.cc | 19 +- pc/peerconnection.h | 3 +- pc/peerconnection_rtp_unittest.cc | 165 ++++++++++++++++++ pc/test/mockpeerconnectionobservers.h | 33 +++- ...eerconnection_unittests.gtest-memcheck.txt | 5 +- 7 files changed, 235 insertions(+), 16 deletions(-) create mode 100644 pc/peerconnection_rtp_unittest.cc 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.*