From 00cc836fcfa031a16d9c62375d5aa490519c3ac6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Mon, 25 Nov 2019 12:21:46 +0100 Subject: [PATCH] Makes all of RtpVideoSenderTest use simulated time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RtpVideoSenderTest used a SimulatedClock but the task queue factor still looked at the real-time clock when posting delayed tasks. This CL changes that so everything is using simulated time, which makes test faster and should avoid flakiness. In particular, fixing this timing issue exposed flaws in DoesNotRetrasmitAckedPackets, which was likely the root case of bug 10873, so let's re-enable on ios again. Bug: webrtc:10873,webrtc:10809 Change-Id: If8a0c244b1a34f7427543deaa2431ab1e9f124a6 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/160404 Reviewed-by: Niels Moller Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#29897} --- call/BUILD.gn | 1 + call/rtp_video_sender_unittest.cc | 65 ++++++++++++++----------------- 2 files changed, 31 insertions(+), 35 deletions(-) diff --git a/call/BUILD.gn b/call/BUILD.gn index 882ea147f7..26a0b377ce 100644 --- a/call/BUILD.gn +++ b/call/BUILD.gn @@ -421,6 +421,7 @@ if (rtc_include_tests) { "../test:test_common", "../test:test_support", "../test:video_test_common", + "../test/time_controller:time_controller", "../video", "//testing/gmock", "//testing/gtest", diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index d453f45211..8ea4124082 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc @@ -12,8 +12,6 @@ #include #include - -#include "api/task_queue/default_task_queue_factory.h" #include "call/rtp_transport_controller_send.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/byte_io.h" @@ -27,6 +25,7 @@ #include "test/gmock.h" #include "test/gtest.h" #include "test/mock_transport.h" +#include "test/time_controller/simulated_time_controller.h" #include "video/call_stats.h" #include "video/send_delay_stats.h" #include "video/send_statistics_proxy.h" @@ -114,39 +113,39 @@ class RtpVideoSenderTestFixture { int payload_type, const std::map& suspended_payload_states, FrameCountObserver* frame_count_observer) - : clock_(1000000), + : time_controller_(Timestamp::ms(1000000)), config_(CreateVideoSendStreamConfig(&transport_, ssrcs, rtx_ssrcs, payload_type)), - send_delay_stats_(&clock_), + send_delay_stats_(time_controller_.GetClock()), bitrate_config_(GetBitrateConfig()), - task_queue_factory_(CreateDefaultTaskQueueFactory()), - transport_controller_(&clock_, + transport_controller_(time_controller_.GetClock(), &event_log_, nullptr, nullptr, bitrate_config_, ProcessThread::Create("PacerThread"), - task_queue_factory_.get(), + time_controller_.GetTaskQueueFactory(), &field_trials_), process_thread_(ProcessThread::Create("test_thread")), - call_stats_(&clock_, process_thread_.get()), - stats_proxy_(&clock_, + call_stats_(time_controller_.GetClock(), process_thread_.get()), + stats_proxy_(time_controller_.GetClock(), config_, VideoEncoderConfig::ContentType::kRealtimeVideo), - retransmission_rate_limiter_(&clock_, kRetransmitWindowSizeMs) { + retransmission_rate_limiter_(time_controller_.GetClock(), + kRetransmitWindowSizeMs) { std::map suspended_ssrcs; router_ = std::make_unique( - &clock_, suspended_ssrcs, suspended_payload_states, config_.rtp, - config_.rtcp_report_interval_ms, &transport_, + time_controller_.GetClock(), suspended_ssrcs, suspended_payload_states, + config_.rtp, config_.rtcp_report_interval_ms, &transport_, CreateObservers(&call_stats_, &encoder_feedback_, &stats_proxy_, &stats_proxy_, &stats_proxy_, &stats_proxy_, frame_count_observer, &stats_proxy_, &stats_proxy_, &send_delay_stats_), &transport_controller_, &event_log_, &retransmission_rate_limiter_, - std::make_unique(&clock_), nullptr, - CryptoOptions{}); + std::make_unique(time_controller_.GetClock()), + nullptr, CryptoOptions{}); } RtpVideoSenderTestFixture( const std::vector& ssrcs, @@ -161,17 +160,16 @@ class RtpVideoSenderTestFixture { RtpVideoSender* router() { return router_.get(); } MockTransport& transport() { return transport_; } - SimulatedClock& clock() { return clock_; } + void AdvanceTime(TimeDelta delta) { time_controller_.Sleep(delta); } private: NiceMock transport_; NiceMock encoder_feedback_; - SimulatedClock clock_; + GlobalSimulatedTimeController time_controller_; RtcEventLogNull event_log_; VideoSendStream::Config config_; SendDelayStats send_delay_stats_; BitrateConstraints bitrate_config_; - const std::unique_ptr task_queue_factory_; const FieldTrialBasedConfig field_trials_; RtpTransportControllerSend transport_controller_; std::unique_ptr process_thread_; @@ -390,12 +388,7 @@ TEST(RtpVideoSenderTest, FrameCountCallbacks) { // Integration test verifying that ack of packet via TransportFeedback means // that the packet is removed from RtpPacketHistory and won't be retransmitted // again. -// TODO(crbug.com/webrtc/10873): Re-enable on iOS -#if defined(WEBRTC_IOS) -TEST(RtpVideoSenderTest, DISABLED_DoesNotRetrasmitAckedPackets) { -#else TEST(RtpVideoSenderTest, DoesNotRetrasmitAckedPackets) { -#endif const int64_t kTimeoutMs = 500; RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2}, @@ -437,7 +430,7 @@ TEST(RtpVideoSenderTest, DoesNotRetrasmitAckedPackets) { EncodedImageCallback::Result::OK, test.router()->OnEncodedImage(encoded_image, nullptr, nullptr).error); - test.clock().AdvanceTimeMilliseconds(33); + test.AdvanceTime(TimeDelta::ms(33)); ASSERT_TRUE(event.Wait(kTimeoutMs)); @@ -466,32 +459,33 @@ TEST(RtpVideoSenderTest, DoesNotRetrasmitAckedPackets) { return true; }); test.router()->DeliverRtcp(nack_buffer.data(), nack_buffer.size()); + test.AdvanceTime(TimeDelta::ms(33)); ASSERT_TRUE(event.Wait(kTimeoutMs)); // Verify that both packets were retransmitted. EXPECT_EQ(retransmitted_rtp_sequence_numbers, rtp_sequence_numbers); // Simulate transport feedback indicating fist packet received, next packet - // lost. - StreamFeedbackObserver::StreamPacketInfo received_packet_feedback; - received_packet_feedback.rtp_sequence_number = rtp_sequence_numbers[0]; - received_packet_feedback.ssrc = kSsrc1; - received_packet_feedback.received = true; - + // lost (not other way around as that would trigger early retransmit). StreamFeedbackObserver::StreamPacketInfo lost_packet_feedback; - lost_packet_feedback.rtp_sequence_number = rtp_sequence_numbers[1]; + lost_packet_feedback.rtp_sequence_number = rtp_sequence_numbers[0]; lost_packet_feedback.ssrc = kSsrc1; lost_packet_feedback.received = false; + StreamFeedbackObserver::StreamPacketInfo received_packet_feedback; + received_packet_feedback.rtp_sequence_number = rtp_sequence_numbers[1]; + received_packet_feedback.ssrc = kSsrc1; + received_packet_feedback.received = true; + test.router()->OnPacketFeedbackVector( - {received_packet_feedback, lost_packet_feedback}); + {lost_packet_feedback, received_packet_feedback}); // Advance time to make sure retransmission would be allowed and try again. // This time the retransmission should not happen for the first packet since // the history has been notified of the ack and removed the packet. The // second packet, included in the feedback but not marked as received, should // still be retransmitted. - test.clock().AdvanceTimeMilliseconds(33); + test.AdvanceTime(TimeDelta::ms(33)); EXPECT_CALL(test.transport(), SendRtp) .WillOnce([&event, &lost_packet_feedback](const uint8_t* packet, size_t length, @@ -507,6 +501,7 @@ TEST(RtpVideoSenderTest, DoesNotRetrasmitAckedPackets) { return true; }); test.router()->DeliverRtcp(nack_buffer.data(), nack_buffer.size()); + test.AdvanceTime(TimeDelta::ms(33)); ASSERT_TRUE(event.Wait(kTimeoutMs)); } @@ -554,7 +549,7 @@ TEST(RtpVideoSenderTest, EarlyRetransmits) { .error, EncodedImageCallback::Result::OK); - test.clock().AdvanceTimeMilliseconds(33); + test.AdvanceTime(TimeDelta::ms(33)); ASSERT_TRUE(event.Wait(kTimeoutMs)); uint16_t frame2_rtp_sequence_number = 0; @@ -577,7 +572,7 @@ TEST(RtpVideoSenderTest, EarlyRetransmits) { ->OnEncodedImage(encoded_image, &codec_specific, nullptr) .error, EncodedImageCallback::Result::OK); - test.clock().AdvanceTimeMilliseconds(33); + test.AdvanceTime(TimeDelta::ms(33)); ASSERT_TRUE(event.Wait(kTimeoutMs)); EXPECT_NE(frame1_transport_sequence_number, frame2_transport_sequence_number); @@ -615,7 +610,7 @@ TEST(RtpVideoSenderTest, EarlyRetransmits) { {first_packet_feedback, second_packet_feedback}); // Wait for pacer to run and send the RTX packet. - test.clock().AdvanceTimeMilliseconds(33); + test.AdvanceTime(TimeDelta::ms(33)); ASSERT_TRUE(event.Wait(kTimeoutMs)); }