From be3947020382cc9733a9b53dff064f1353375bb5 Mon Sep 17 00:00:00 2001 From: "henrik.lundin@webrtc.org" Date: Tue, 11 Mar 2014 17:13:14 +0000 Subject: [PATCH] Revert "Routing SuspendChange to VideoSendStream::Stats" The test VideoSendStreamTest.SuspendBelowMinBitrate seems flaky. Reverting and investigating. BUG=3040 Review URL: https://webrtc-codereview.appspot.com/9799004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@5681 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/video/rampup_tests.cc | 19 +++---------------- webrtc/video/send_statistics_proxy.cc | 5 ----- webrtc/video/send_statistics_proxy.h | 9 ++++----- .../video/send_statistics_proxy_unittest.cc | 15 --------------- webrtc/video/video_send_stream_tests.cc | 16 +++------------- webrtc/video_send_stream.h | 4 +--- 6 files changed, 11 insertions(+), 57 deletions(-) diff --git a/webrtc/video/rampup_tests.cc b/webrtc/video/rampup_tests.cc index 6e4138a1a7..3347cc8f1a 100644 --- a/webrtc/video/rampup_tests.cc +++ b/webrtc/video/rampup_tests.cc @@ -217,9 +217,7 @@ class LowRateStreamObserver : public test::DirectTransport, sent_bytes_(0), total_overuse_bytes_(0), number_of_streams_(number_of_streams), - rtx_used_(rtx_used), - send_stream_(NULL), - suspended_in_stats_(true) { + rtx_used_(rtx_used) { RtpRtcp::Configuration config; config.receive_statistics = receive_stats_.get(); feedback_transport_.Enable(); @@ -240,10 +238,6 @@ class LowRateStreamObserver : public test::DirectTransport, test::DirectTransport::SetReceiver(this); } - virtual void SetSendStream(const VideoSendStream* send_stream) { - send_stream_ = send_stream; - } - virtual void OnReceiveBitrateChanged(const std::vector& ssrcs, unsigned int bitrate) { CriticalSectionScoped lock(critical_section_.get()); @@ -284,7 +278,6 @@ class LowRateStreamObserver : public test::DirectTransport, if (remote_bitrate_estimator_->TimeUntilNextProcess() <= 0) { remote_bitrate_estimator_->Process(); } - suspended_in_stats_ = send_stream_->GetStats().suspended; return true; } @@ -310,11 +303,8 @@ class LowRateStreamObserver : public test::DirectTransport, // This method defines the state machine for the ramp up-down-up test. void EvolveTestState(unsigned int bitrate_bps) { int64_t now = clock_->TimeInMilliseconds(); - assert(send_stream_ != NULL); - CriticalSectionScoped lock(critical_section_.get()); switch (test_state_) { case kFirstRampup: { - EXPECT_FALSE(suspended_in_stats_); if (bitrate_bps > kExpectedHighBitrateBps) { // The first ramp-up has reached the target bitrate. Change the // channel limit, and move to the next test state. @@ -335,7 +325,7 @@ class LowRateStreamObserver : public test::DirectTransport, break; } case kLowRate: { - if (bitrate_bps < kExpectedLowBitrateBps && suspended_in_stats_) { + if (bitrate_bps < kExpectedLowBitrateBps) { // The ramp-down was successful. Change the channel limit back to a // high value, and move to the next test state. forward_transport_config_.link_capacity_kbps = @@ -355,7 +345,7 @@ class LowRateStreamObserver : public test::DirectTransport, break; } case kSecondRampup: { - if (bitrate_bps > kExpectedHighBitrateBps && !suspended_in_stats_) { + if (bitrate_bps > kExpectedHighBitrateBps) { webrtc::test::PrintResult("ramp_up_down_up", GetModifierString(), "second_rampup", @@ -401,8 +391,6 @@ class LowRateStreamObserver : public test::DirectTransport, size_t total_overuse_bytes_; const size_t number_of_streams_; const bool rtx_used_; - const VideoSendStream* send_stream_; - bool suspended_in_stats_ GUARDED_BY(critical_section_); }; } @@ -508,7 +496,6 @@ class RampUpTest : public ::testing::Test { send_config.suspend_below_min_bitrate = true; VideoSendStream* send_stream = call->CreateVideoSendStream(send_config); - stream_observer.SetSendStream(send_stream); scoped_ptr frame_generator_capturer( test::FrameGeneratorCapturer::Create(send_stream->Input(), diff --git a/webrtc/video/send_statistics_proxy.cc b/webrtc/video/send_statistics_proxy.cc index 1cd4e26afc..096f0a92ad 100644 --- a/webrtc/video/send_statistics_proxy.cc +++ b/webrtc/video/send_statistics_proxy.cc @@ -32,11 +32,6 @@ void SendStatisticsProxy::OutgoingRate(const int video_channel, stats_.encode_frame_rate = framerate; } -void SendStatisticsProxy::SuspendChange(int video_channel, bool is_suspended) { - CriticalSectionScoped cs(lock_.get()); - stats_.suspended = is_suspended; -} - void SendStatisticsProxy::CapturedFrameRate(const int capture_id, const unsigned char frame_rate) { CriticalSectionScoped cs(lock_.get()); diff --git a/webrtc/video/send_statistics_proxy.h b/webrtc/video/send_statistics_proxy.h index 2f45ff7f9b..5ad4c4550d 100644 --- a/webrtc/video/send_statistics_proxy.h +++ b/webrtc/video/send_statistics_proxy.h @@ -18,7 +18,6 @@ #include "webrtc/video_engine/include/vie_capture.h" #include "webrtc/video_send_stream.h" #include "webrtc/system_wrappers/interface/scoped_ptr.h" -#include "webrtc/system_wrappers/interface/thread_annotations.h" namespace webrtc { @@ -68,7 +67,7 @@ class SendStatisticsProxy : public RtcpStatisticsCallback, const unsigned int framerate, const unsigned int bitrate) OVERRIDE; - virtual void SuspendChange(int video_channel, bool is_suspended) OVERRIDE; + virtual void SuspendChange(int video_channel, bool is_suspended) OVERRIDE {} // From ViECaptureObserver. virtual void BrightnessAlarm(const int capture_id, @@ -81,12 +80,12 @@ class SendStatisticsProxy : public RtcpStatisticsCallback, const CaptureAlarm alarm) OVERRIDE {} private: - StreamStats* GetStatsEntry(uint32_t ssrc) EXCLUSIVE_LOCKS_REQUIRED(lock_); + StreamStats* GetStatsEntry(uint32_t ssrc); const VideoSendStream::Config config_; scoped_ptr lock_; - VideoSendStream::Stats stats_ GUARDED_BY(lock_); - StatsProvider* const stats_provider_; + VideoSendStream::Stats stats_; + StatsProvider* stats_provider_; }; } // namespace webrtc diff --git a/webrtc/video/send_statistics_proxy_unittest.cc b/webrtc/video/send_statistics_proxy_unittest.cc index 8f35ee4d03..ed74a4f775 100644 --- a/webrtc/video/send_statistics_proxy_unittest.cc +++ b/webrtc/video/send_statistics_proxy_unittest.cc @@ -54,7 +54,6 @@ class SendStatisticsProxyTest : public ::testing::Test, EXPECT_EQ(one.encode_frame_rate, other.encode_frame_rate); EXPECT_EQ(one.avg_delay_ms, other.avg_delay_ms); EXPECT_EQ(one.max_delay_ms, other.max_delay_ms); - EXPECT_EQ(one.suspended, other.suspended); EXPECT_EQ(one.c_name, other.c_name); EXPECT_EQ(one.substreams.size(), other.substreams.size()); @@ -132,20 +131,6 @@ TEST_F(SendStatisticsProxyTest, FrameRates) { EXPECT_EQ(encode_fps, stats.encode_frame_rate); } -TEST_F(SendStatisticsProxyTest, Suspended) { - // Verify that the value is false by default. - EXPECT_FALSE(statistics_proxy_->GetStats().suspended); - - // Verify that we can set it to true. - ViEEncoderObserver* encoder_observer = statistics_proxy_.get(); - encoder_observer->SuspendChange(0, true); - EXPECT_TRUE(statistics_proxy_->GetStats().suspended); - - // Verify that we can set it back to false again. - encoder_observer->SuspendChange(0, false); - EXPECT_FALSE(statistics_proxy_->GetStats().suspended); -} - TEST_F(SendStatisticsProxyTest, FrameCounts) { FrameCountObserver* observer = statistics_proxy_.get(); for (std::vector::const_iterator it = config_.rtp.ssrcs.begin(); diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index dd1bd76ad8..41243e6404 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -876,7 +876,7 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { class RembObserver : public test::RtpRtcpObserver, public I420FrameCallback { public: - RembObserver(VideoSendStream** send_stream_ptr) + RembObserver() : RtpRtcpObserver(30 * 1000), // Timeout after 30 seconds. transport_adapter_(&transport_), clock_(Clock::GetRealTimeClock()), @@ -886,8 +886,7 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { suspended_frame_count_(0), low_remb_bps_(0), high_remb_bps_(0), - crit_sect_(CriticalSectionWrapper::CreateCriticalSection()), - send_stream_ptr_(send_stream_ptr) { + crit_sect_(CriticalSectionWrapper::CreateCriticalSection()) { transport_adapter_.Enable(); } @@ -915,9 +914,6 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { SendRtcpFeedback(low_remb_bps_); test_state_ = kDuringSuspend; } else if (test_state_ == kDuringSuspend) { - assert(*send_stream_ptr_); - VideoSendStream::Stats stats = (*send_stream_ptr_)->GetStats(); - EXPECT_TRUE(stats.suspended); if (header.paddingLength == 0) { // Received non-padding packet during suspension period. Reset the // counter. @@ -930,9 +926,6 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { } else if (test_state_ == kWaitingForPacket) { if (header.paddingLength == 0) { // Non-padding packet observed. Test is complete. - assert(*send_stream_ptr_); - VideoSendStream::Stats stats = (*send_stream_ptr_)->GetStats(); - EXPECT_FALSE(stats.suspended); observation_complete_->Set(); } } @@ -990,10 +983,7 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { int low_remb_bps_; int high_remb_bps_; scoped_ptr crit_sect_; - VideoSendStream** send_stream_ptr_; - } observer(&send_stream_); - // Note that |send_stream_| is created in RunSendTest(), called below. This - // is why a pointer to |send_stream_| must be provided here. + } observer; Call::Config call_config(observer.SendTransport()); scoped_ptr call(Call::Create(call_config)); diff --git a/webrtc/video_send_stream.h b/webrtc/video_send_stream.h index a6f7e63b47..c3140c076a 100644 --- a/webrtc/video_send_stream.h +++ b/webrtc/video_send_stream.h @@ -43,14 +43,12 @@ class VideoSendStream { : input_frame_rate(0), encode_frame_rate(0), avg_delay_ms(0), - max_delay_ms(0), - suspended(false) {} + max_delay_ms(0) {} int input_frame_rate; int encode_frame_rate; int avg_delay_ms; int max_delay_ms; - bool suspended; std::string c_name; std::map substreams; };