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}

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<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() {}

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",

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<RtpReceiverInterface> 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<RtpReceiverInterface> 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<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().
-    DestroyReceiver(track_id);
+    receiver = RemoveAndStopReceiver(track_id);
     rtc::scoped_refptr<AudioTrackInterface> 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<VideoTrackInterface> 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() {

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<RtpReceiverInterface> 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.

pc/peerconnection_rtp_unittest.cc

@@ -0,0 +1,195 @@
+/*
+ *  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

pc/test/mockpeerconnectionobservers.h

@@ -25,6 +25,16 @@ namespace webrtc {
 
 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) {
@@ -92,13 +102,26 @@ class MockPeerConnectionObserver : public PeerConnectionObserver {
     callback_triggered_ = true;
   }
 
-  void OnAddTrack(
-      rtc::scoped_refptr<webrtc::RtpReceiverInterface> receiver,
-      const std::vector<rtc::scoped_refptr<webrtc::MediaStreamInterface>>&
-          streams) override {
+  void OnAddTrack(rtc::scoped_refptr<RtpReceiverInterface> receiver,
+                  const std::vector<rtc::scoped_refptr<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.
@@ -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<AddTrackEvent> add_track_events_;
+  std::vector<rtc::scoped_refptr<RtpReceiverInterface>> remove_track_events_;
 
  private:
   rtc::scoped_refptr<MediaStreamInterface> last_added_stream_;

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.*