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}diff --git a/ortc/ortcfactory_integrationtest.cc b/ortc/ortcfactory_integrationtest.cc
index 6a697cd..5a61680 100644
--- a/ortc/ortcfactory_integrationtest.cc
+++ b/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 @@
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>(
- fake_video_sources_.back().get(), false /* remote */));
+ new rtc::RefCountedObject<FakePeriodicVideoTrackSource>(
+ false /* remote */));
return rtc::scoped_refptr<VideoTrackInterface>(
ortc_factory->CreateVideoTrack(
id, fake_video_track_sources_.back()));
@@ -352,7 +350,6 @@
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 @@
// 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()));
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 2942d11..2c564cc 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -354,6 +354,7 @@
"test/fakepeerconnectionforstats.h",
"test/fakeperiodicvideocapturer.h",
"test/fakeperiodicvideosource.h",
+ "test/fakeperiodicvideotracksource.h",
"test/fakertccertificategenerator.h",
"test/fakesctptransport.h",
"test/fakevideotrackrenderer.h",
diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc
index e697e3a..28b7d16 100644
--- a/pc/peerconnection_integrationtest.cc
+++ b/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 @@
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 @@
// 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>(
- fake_video_sources_.back().get(), false /* remote */));
+ new rtc::RefCountedObject<webrtc::FakePeriodicVideoTrackSource>(
+ config, false /* remote */));
rtc::scoped_refptr<webrtc::VideoTrackInterface> track(
peer_connection_factory_->CreateVideoTrack(
rtc::CreateRandomUuid(), video_track_sources_.back()));
@@ -940,8 +923,6 @@
// 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.
diff --git a/pc/test/fakeperiodicvideotracksource.h b/pc/test/fakeperiodicvideotracksource.h
new file mode 100644
index 0000000..1be347c
--- /dev/null
+++ b/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_
