From 0f405825c77bb20513e96f2b4ff4f9d6b2b6e22e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Thu, 17 May 2018 09:16:41 +0200 Subject: [PATCH] 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 Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#23320} --- ortc/ortcfactory_integrationtest.cc | 14 +++------ pc/BUILD.gn | 1 + pc/peerconnection_integrationtest.cc | 25 ++-------------- pc/test/fakeperiodicvideotracksource.h | 41 ++++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 32 deletions(-) create mode 100644 pc/test/fakeperiodicvideotracksource.h diff --git a/ortc/ortcfactory_integrationtest.cc b/ortc/ortcfactory_integrationtest.cc index 6a697cd7e2..5a61680bec 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 @@ 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 CreateLocalVideoTrackAndFakeSource(const std::string& id, OrtcFactoryInterface* ortc_factory) { - fake_video_sources_.emplace_back( - rtc::MakeUnique()); fake_video_track_sources_.emplace_back( - new rtc::RefCountedObject( - fake_video_sources_.back().get(), false /* remote */)); + new rtc::RefCountedObject( + false /* remote */)); return rtc::scoped_refptr( ortc_factory->CreateVideoTrack( id, fake_video_track_sources_.back())); @@ -352,7 +350,6 @@ class OrtcFactoryIntegrationTest : public testing::Test { rtc::scoped_refptr fake_audio_capture_module2_; std::unique_ptr ortc_factory1_; std::unique_ptr ortc_factory2_; - std::vector> fake_video_sources_; std::vector> 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())); diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 2942d11714..2c564cc85f 100644 --- a/pc/BUILD.gn +++ b/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", diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index e697e3aef9..28b7d16b91 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 @@ 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(config)); - video_track_sources_.emplace_back( - new rtc::RefCountedObject( - fake_video_sources_.back().get(), false /* remote */)); + new rtc::RefCountedObject( + config, false /* remote */)); rtc::scoped_refptr 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> - fake_video_sources_; std::vector> 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 0000000000..1be347c084 --- /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_