From 6c0c55c31817ecfa32409424495eb68b31828c40 Mon Sep 17 00:00:00 2001 From: Alex Loiko Date: Tue, 10 Oct 2017 11:01:45 +0000 Subject: [PATCH] Revert "Added PeerConnectionObserver::OnRemoveTrack." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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} --- api/peerconnectioninterface.h | 24 +-- pc/BUILD.gn | 2 - pc/peerconnection.cc | 19 +- pc/peerconnection.h | 3 +- pc/peerconnection_rtp_unittest.cc | 195 ------------------ pc/test/mockpeerconnectionobservers.h | 33 +-- ...eerconnection_unittests.gtest-memcheck.txt | 5 +- 7 files changed, 16 insertions(+), 265 deletions(-) delete mode 100644 pc/peerconnection_rtp_unittest.cc diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index 1039cb5a90..7291a8641c 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -868,31 +868,13 @@ class PeerConnectionObserver { // Called when the ICE connection receiving status changes. virtual void OnIceConnectionReceivingChange(bool receiving) {} - // This is called when a receiver and its track is created. - // TODO(zhihuang): Make this pure virtual when all subclasses implement it. + // Called when a track is added to streams. + // TODO(zhihuang) Make this a pure virtual method when all its 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 950a6ee978..6e95bf8a8f 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -393,7 +393,6 @@ 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", @@ -468,7 +467,6 @@ 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 e8e428dd4f..bc95895a8e 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1462,18 +1462,15 @@ void PeerConnection::CreateVideoReceiver(MediaStreamInterface* stream, // TODO(deadbeef): Keep RtpReceivers around even if track goes away in remote // description. -rtc::scoped_refptr PeerConnection::RemoveAndStopReceiver( - const std::string& track_id) { +void PeerConnection::DestroyReceiver(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."; - return nullptr; + } else { + (*it)->internal()->Stop(); + receivers_.erase(it); } - (*it)->internal()->Stop(); - rtc::scoped_refptr receiver = *it; - receivers_.erase(it); - return receiver; } void PeerConnection::AddAudioTrack(AudioTrackInterface* track, @@ -2015,11 +2012,10 @@ 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(). - receiver = RemoveAndStopReceiver(track_id); + DestroyReceiver(track_id); rtc::scoped_refptr audio_track = stream->FindAudioTrack(track_id); if (audio_track) { @@ -2028,7 +2024,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(). - receiver = RemoveAndStopReceiver(track_id); + DestroyReceiver(track_id); rtc::scoped_refptr video_track = stream->FindVideoTrack(track_id); if (video_track) { @@ -2039,9 +2035,6 @@ 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 9d49f934b5..fa06c3d78d 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -246,8 +246,7 @@ class PeerConnection : public PeerConnectionInterface, void CreateVideoReceiver(MediaStreamInterface* stream, const std::string& track_id, uint32_t ssrc); - rtc::scoped_refptr RemoveAndStopReceiver( - const std::string& track_id); + void DestroyReceiver(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 deleted file mode 100644 index 9dd6796d79..0000000000 --- a/pc/peerconnection_rtp_unittest.cc +++ /dev/null @@ -1,195 +0,0 @@ -/* - * 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 { - -// TODO(hbos): Consolidate fake track classes. https://crbug.com/webrtc/8369 -class FakeAudioMediaStreamTrack - : public rtc::RefCountedObject< - webrtc::MediaStreamTrack> { - public: - explicit FakeAudioMediaStreamTrack(const std::string& id) - : rtc::RefCountedObject< - webrtc::MediaStreamTrack>(id) {} - - std::string kind() const override { - return webrtc::MediaStreamTrackInterface::kAudioKind; - } - - webrtc::AudioSourceInterface* GetSource() const override { return nullptr; } - - void AddSink(webrtc::AudioTrackSinkInterface* sink) override {} - - void RemoveSink(webrtc::AudioTrackSinkInterface* sink) override {} - - bool GetSignalLevel(int* level) override { - RTC_NOTREACHED(); - return false; - } - - rtc::scoped_refptr GetAudioProcessor() - override { - RTC_NOTREACHED(); - return nullptr; - } -}; - -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( - new FakeAudioMediaStreamTrack("audio_track")); - 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( - new FakeAudioMediaStreamTrack("audio_track")); - 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( - new FakeAudioMediaStreamTrack("audio_track")); - 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( - new FakeAudioMediaStreamTrack("audio_track")); - 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( - new FakeAudioMediaStreamTrack("audio_track1")); - rtc::scoped_refptr audio_track2( - new FakeAudioMediaStreamTrack("audio_track2")); - 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 119c32b0f5..64f84b6b8e 100644 --- a/pc/test/mockpeerconnectionobservers.h +++ b/pc/test/mockpeerconnectionobservers.h @@ -25,16 +25,6 @@ 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) { @@ -102,26 +92,13 @@ 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. @@ -147,8 +124,6 @@ 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 8f10317aae..497f1f0b8f 100644 --- a/tools_webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt +++ b/tools_webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt @@ -1,9 +1,8 @@ # Tests that are failing when run under memcheck. # https://code.google.com/p/webrtc/issues/detail?id=4387 DtmfSenderTest.* -PeerConnectionEndToEndTest.* -PeerConnectionCryptoUnitTest.* PeerConnectionIntegrationTest.* +PeerConnectionEndToEndTest.* PeerConnectionInterfaceTest.* -PeerConnectionRtpTest.* +PeerConnectionCryptoUnitTest.* RTCStatsIntegrationTest.*