New class FakePeriodicVideoTrackSource, simplifying shutdown logic.

Previous code had a FakePeriodicVideoSource and a
VideoTrackSource, where the latter is reference counted and
outlives the former. That results in potential races when
RemoveSink is called on the VideoTrackSource after the
FakePeriodicVideoSource is destroyed, with a complicated sequence
to do correct shutdown.

The new class, FakePeriodicVideoTrackSource, owns a
FakePeriodicVideoSource, and they get the same lifetime.

Bug: webrtc:6353
Change-Id: Ic33b393e00a31fa28893dce2018948d3f90e0a9e
Reviewed-on: https://webrtc-review.googlesource.com/76961
Commit-Queue: Niels Moller <nisse@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23320}

ortc/ortcfactory_integrationtest.cc

@@ -17,7 +17,7 @@
 #include "ortc/testrtpparameters.h"
 #include "p2p/base/udptransport.h"
 #include "pc/test/fakeaudiocapturemodule.h"
-#include "pc/test/fakeperiodicvideosource.h"
+#include "pc/test/fakeperiodicvideotracksource.h"
 #include "pc/test/fakevideotrackrenderer.h"
 #include "pc/videotracksource.h"
 #include "rtc_base/criticalsection.h"
@@ -219,15 +219,13 @@ class OrtcFactoryIntegrationTest : public testing::Test {
     return ortc_factory->CreateAudioTrack(id, source);
   }
 
-  // Stores created video source in |fake_video_sources_|.
+  // Stores created video source in |fake_video_track_sources_|.
   rtc::scoped_refptr<webrtc::VideoTrackInterface>
   CreateLocalVideoTrackAndFakeSource(const std::string& id,
                                      OrtcFactoryInterface* ortc_factory) {
-    fake_video_sources_.emplace_back(
-        rtc::MakeUnique<FakePeriodicVideoSource>());
     fake_video_track_sources_.emplace_back(
-        new rtc::RefCountedObject<VideoTrackSource>(
+        new rtc::RefCountedObject<FakePeriodicVideoTrackSource>(
-            fake_video_sources_.back().get(), false /* remote */));
+            false /* remote */));
     return rtc::scoped_refptr<VideoTrackInterface>(
         ortc_factory->CreateVideoTrack(
             id, fake_video_track_sources_.back()));
@@ -352,7 +350,6 @@ class OrtcFactoryIntegrationTest : public testing::Test {
   rtc::scoped_refptr<FakeAudioCaptureModule> fake_audio_capture_module2_;
   std::unique_ptr<OrtcFactoryInterface> ortc_factory1_;
   std::unique_ptr<OrtcFactoryInterface> ortc_factory2_;
-  std::vector<std::unique_ptr<FakePeriodicVideoSource>> fake_video_sources_;
   std::vector<rtc::scoped_refptr<VideoTrackSource>> fake_video_track_sources_;
   int received_audio_frames1_ = 0;
   int received_audio_frames2_ = 0;
@@ -465,10 +462,7 @@ TEST_F(OrtcFactoryIntegrationTest, SetTrackWhileSending) {
   // Destroy old source, set a new track, and verify new frames are received
   // from the new track. The VideoTrackSource is reference counted and may live
   // a little longer, so tell it that its source is going away now.
-  fake_video_sources_[0]->Stop();
-  fake_video_track_sources_[0]->OnSourceDestroyed();
   fake_video_track_sources_[0] = nullptr;
-  fake_video_sources_[0].reset();
   int prev_num_frames = fake_renderer.num_rendered_frames();
   error = sender->SetTrack(
       CreateLocalVideoTrackAndFakeSource("video_2", ortc_factory1_.get()));

pc/BUILD.gn

@@ -354,6 +354,7 @@ if (rtc_include_tests) {
       "test/fakepeerconnectionforstats.h",
       "test/fakeperiodicvideocapturer.h",
       "test/fakeperiodicvideosource.h",
+      "test/fakeperiodicvideotracksource.h",
       "test/fakertccertificategenerator.h",
       "test/fakesctptransport.h",
       "test/fakevideotrackrenderer.h",

pc/peerconnection_integrationtest.cc

@@ -48,7 +48,7 @@
 #include "pc/rtpmediautils.h"
 #include "pc/sessiondescription.h"
 #include "pc/test/fakeaudiocapturemodule.h"
-#include "pc/test/fakeperiodicvideosource.h"
+#include "pc/test/fakeperiodicvideotracksource.h"
 #include "pc/test/fakertccertificategenerator.h"
 #include "pc/test/fakevideotrackrenderer.h"
 #include "pc/test/mockpeerconnectionobservers.h"
@@ -238,20 +238,6 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver,
     return client;
   }
 
-  ~PeerConnectionWrapper() {
-    // Tear down video sources in the proper order.
-    for (const auto& video_source : fake_video_sources_) {
-      // No more calls to downstream OnFrame
-      video_source->Stop();
-    }
-    for (const auto& track_source : video_track_sources_) {
-      // No more calls to upstream AddOrUpdateSink
-      track_source->OnSourceDestroyed();
-    }
-    fake_video_sources_.clear();
-    video_track_sources_.clear();
-  }
-
   webrtc::PeerConnectionFactoryInterface* pc_factory() const {
     return peer_connection_factory_.get();
   }
@@ -655,12 +641,9 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver,
     // TODO(deadbeef): Do something more robust.
     config.frame_interval_ms = 100;
 
-    fake_video_sources_.emplace_back(
-        rtc::MakeUnique<webrtc::FakePeriodicVideoSource>(config));
-
     video_track_sources_.emplace_back(
-        new rtc::RefCountedObject<webrtc::VideoTrackSource>(
+        new rtc::RefCountedObject<webrtc::FakePeriodicVideoTrackSource>(
-            fake_video_sources_.back().get(), false /* remote */));
+            config, false /* remote */));
     rtc::scoped_refptr<webrtc::VideoTrackInterface> track(
         peer_connection_factory_->CreateVideoTrack(
             rtc::CreateRandomUuid(), video_track_sources_.back()));
@@ -940,8 +923,6 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver,
 
   // Store references to the video sources we've created, so that we can stop
   // them, if required.
-  std::vector<std::unique_ptr<webrtc::FakePeriodicVideoSource>>
-      fake_video_sources_;
   std::vector<rtc::scoped_refptr<webrtc::VideoTrackSource>>
       video_track_sources_;
   // |local_video_renderer_| attached to the first created local video track.

pc/test/fakeperiodicvideotracksource.h

@@ -0,0 +1,41 @@
+/*
+ *  Copyright 2018 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.
+ */
+
+#ifndef PC_TEST_FAKEPERIODICVIDEOTRACKSOURCE_H_
+#define PC_TEST_FAKEPERIODICVIDEOTRACKSOURCE_H_
+
+#include "pc/videotracksource.h"
+#include "pc/test/fakeperiodicvideosource.h"
+
+namespace webrtc {
+
+// A VideoTrackSource generating frames with configured size and frame interval.
+class FakePeriodicVideoTrackSource : public VideoTrackSource {
+ public:
+  explicit FakePeriodicVideoTrackSource(bool remote)
+      : FakePeriodicVideoTrackSource(FakePeriodicVideoSource::Config(),
+                                     remote) {}
+
+  FakePeriodicVideoTrackSource(FakePeriodicVideoSource::Config config,
+                               bool remote)
+      // Note that VideoTrack constructor gets a pointer to an
+      // uninitialized source object; that works because it only
+      // stores the pointer for later use.
+      : VideoTrackSource(&source_, remote), source_(config) {}
+
+  ~FakePeriodicVideoTrackSource() = default;
+
+ private:
+  FakePeriodicVideoSource source_;
+};
+
+}  // namespace webrtc
+
+#endif  // PC_TEST_FAKEPERIODICVIDEOTRACKSOURCE_H_