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 <hbos@webrtc.org>
> Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
> Reviewed-by: Steve Anton <steveanton@webrtc.org>
> 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 <aleloi@webrtc.org>
Commit-Queue: Alex Loiko <aleloi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20218}diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h
index 1039cb5..7291a86 100644
--- a/api/peerconnectioninterface.h
+++ b/api/peerconnectioninterface.h
@@ -868,31 +868,13 @@
// 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<RtpReceiverInterface> receiver,
const std::vector<rtc::scoped_refptr<MediaStreamInterface>>& 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<RtpReceiverInterface> 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 950a6ee..6e95bf8 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -393,7 +393,6 @@
"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 @@
"../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 e8e428d..bc95895 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -1462,18 +1462,15 @@
// TODO(deadbeef): Keep RtpReceivers around even if track goes away in remote
// description.
-rtc::scoped_refptr<RtpReceiverInterface> 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<RtpReceiverInterface> receiver = *it;
- receivers_.erase(it);
- return receiver;
}
void PeerConnection::AddAudioTrack(AudioTrackInterface* track,
@@ -2015,11 +2012,10 @@
cricket::MediaType media_type) {
MediaStreamInterface* stream = remote_streams_->find(stream_label);
- rtc::scoped_refptr<RtpReceiverInterface> 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<AudioTrackInterface> audio_track =
stream->FindAudioTrack(track_id);
if (audio_track) {
@@ -2028,7 +2024,7 @@
} 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<VideoTrackInterface> video_track =
stream->FindVideoTrack(track_id);
if (video_track) {
@@ -2039,9 +2035,6 @@
} 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 9d49f93..fa06c3d 100644
--- a/pc/peerconnection.h
+++ b/pc/peerconnection.h
@@ -246,8 +246,7 @@
void CreateVideoReceiver(MediaStreamInterface* stream,
const std::string& track_id,
uint32_t ssrc);
- rtc::scoped_refptr<RtpReceiverInterface> 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 9dd6796..0000000
--- 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 <memory>
-#include <vector>
-
-#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<webrtc::AudioTrackInterface>> {
- public:
- explicit FakeAudioMediaStreamTrack(const std::string& id)
- : rtc::RefCountedObject<
- webrtc::MediaStreamTrack<webrtc::AudioTrackInterface>>(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<webrtc::AudioProcessorInterface> 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<webrtc::PeerConnectionWrapper> CreatePeerConnection() {
- webrtc::PeerConnectionInterface::RTCConfiguration config;
- auto observer = rtc::MakeUnique<webrtc::MockPeerConnectionObserver>();
- auto pc = pc_factory_->CreatePeerConnection(config, nullptr, nullptr,
- observer.get());
- return std::unique_ptr<webrtc::PeerConnectionWrapper>(
- new webrtc::PeerConnectionWrapper(pc_factory_, pc,
- std::move(observer)));
- }
-
- protected:
- rtc::scoped_refptr<webrtc::PeerConnectionFactoryInterface> pc_factory_;
-};
-
-TEST_F(PeerConnectionRtpTest, AddTrackWithoutStreamFiresOnAddTrack) {
- auto caller = CreatePeerConnection();
- auto callee = CreatePeerConnection();
-
- rtc::scoped_refptr<FakeAudioMediaStreamTrack> 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<FakeAudioMediaStreamTrack> 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<FakeAudioMediaStreamTrack> 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<FakeAudioMediaStreamTrack> 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<FakeAudioMediaStreamTrack> audio_track1(
- new FakeAudioMediaStreamTrack("audio_track1"));
- rtc::scoped_refptr<FakeAudioMediaStreamTrack> audio_track2(
- new FakeAudioMediaStreamTrack("audio_track2"));
- auto stream = webrtc::MediaStream::Create("shared_audio_stream");
- std::vector<webrtc::MediaStreamInterface*> 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<rtc::scoped_refptr<webrtc::RtpReceiverInterface>>{
- 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 119c32b..64f84b6 100644
--- a/pc/test/mockpeerconnectionobservers.h
+++ b/pc/test/mockpeerconnectionobservers.h
@@ -25,16 +25,6 @@
class MockPeerConnectionObserver : public PeerConnectionObserver {
public:
- struct AddTrackEvent {
- explicit AddTrackEvent(
- rtc::scoped_refptr<RtpReceiverInterface> receiver,
- std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams)
- : receiver(std::move(receiver)), streams(std::move(streams)) {}
-
- rtc::scoped_refptr<RtpReceiverInterface> receiver;
- std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams;
- };
-
MockPeerConnectionObserver() : remote_streams_(StreamCollection::Create()) {}
virtual ~MockPeerConnectionObserver() {}
void SetPeerConnectionInterface(PeerConnectionInterface* pc) {
@@ -102,26 +92,13 @@
callback_triggered_ = true;
}
- void OnAddTrack(rtc::scoped_refptr<RtpReceiverInterface> receiver,
- const std::vector<rtc::scoped_refptr<MediaStreamInterface>>&
- streams) override {
+ void OnAddTrack(
+ rtc::scoped_refptr<webrtc::RtpReceiverInterface> receiver,
+ const std::vector<rtc::scoped_refptr<webrtc::MediaStreamInterface>>&
+ 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<RtpReceiverInterface> receiver) override {
- remove_track_events_.push_back(receiver);
- }
-
- std::vector<rtc::scoped_refptr<RtpReceiverInterface>> GetAddTrackReceivers() {
- std::vector<rtc::scoped_refptr<RtpReceiverInterface>> 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 @@
bool callback_triggered_ = false;
int num_added_tracks_ = 0;
std::string last_added_track_label_;
- std::vector<AddTrackEvent> add_track_events_;
- std::vector<rtc::scoped_refptr<RtpReceiverInterface>> remove_track_events_;
private:
rtc::scoped_refptr<MediaStreamInterface> 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 8f10317..497f1f0 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.*