From 9c38c47630cd6426b3bef1e7371d4efee789c99b Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Mon, 3 Sep 2018 16:11:42 +0200 Subject: [PATCH] Calculate NtpCaptureStartMs using clock, not the frame rtp timestamp If the first frame rtp timestamp got corrupted somehow, the introduced error would stay there for the duration of the call. Using realtime clock to calculate elapsed time instead of rtp timestamps resolves that problem. The error will go away once ntp time would be estimated correctly from the correct timestamps. Bug: webrtc:9698 Change-Id: Ifa4c3f55f280fae8ec9f1826a89c251ec61b965e Reviewed-on: https://webrtc-review.googlesource.com/97101 Reviewed-by: Fredrik Solenberg Reviewed-by: Stefan Holmer Commit-Queue: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#24726} --- media/engine/webrtcvideoengine.cc | 9 +++------ media/engine/webrtcvideoengine_unittest.cc | 11 ++++++++++- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/media/engine/webrtcvideoengine.cc b/media/engine/webrtcvideoengine.cc index dce6953433..7caf3a4f56 100644 --- a/media/engine/webrtcvideoengine.cc +++ b/media/engine/webrtcvideoengine.cc @@ -2429,13 +2429,10 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::OnFrame( const webrtc::VideoFrame& frame) { rtc::CritScope crit(&sink_lock_); + int64_t time_now_ms = rtc::TimeMillis(); if (first_frame_timestamp_ < 0) - first_frame_timestamp_ = frame.timestamp(); - int64_t rtp_time_elapsed_since_first_frame = - (timestamp_wraparound_handler_.Unwrap(frame.timestamp()) - - first_frame_timestamp_); - int64_t elapsed_time_ms = rtp_time_elapsed_since_first_frame / - (cricket::kVideoCodecClockrate / 1000); + first_frame_timestamp_ = time_now_ms; + int64_t elapsed_time_ms = time_now_ms - first_frame_timestamp_; if (frame.ntp_time_ms() > 0) estimated_remote_start_ntp_time_ms_ = frame.ntp_time_ms() - elapsed_time_ms; diff --git a/media/engine/webrtcvideoengine_unittest.cc b/media/engine/webrtcvideoengine_unittest.cc index 4a45494982..d825078ab3 100644 --- a/media/engine/webrtcvideoengine_unittest.cc +++ b/media/engine/webrtcvideoengine_unittest.cc @@ -40,6 +40,7 @@ #include "media/engine/webrtcvoiceengine.h" #include "modules/rtp_rtcp/include/rtp_header_parser.h" #include "rtc_base/arraysize.h" +#include "rtc_base/fakeclock.h" #include "rtc_base/gunit.h" #include "rtc_base/numerics/safe_conversions.h" #include "rtc_base/stringutils.h" @@ -210,7 +211,11 @@ class WebRtcVideoEngineTest : public ::testing::Test { engine_(std::unique_ptr( encoder_factory_), std::unique_ptr( - decoder_factory_)) {} + decoder_factory_)) { + // Ensure fake clock doesn't return 0, which will cause some initializations + // fail inside RTP senders. + fake_clock_.AdvanceTimeMicros(1); + } protected: void AssignDefaultAptRtxTypes(); @@ -231,6 +236,9 @@ class WebRtcVideoEngineTest : public ::testing::Test { void TestExtendedEncoderOveruse(bool use_external_encoder); + // Has to be the first one, so it is initialized before the call or there is a + // race condition in the clock access. + rtc::ScopedFakeClock fake_clock_; webrtc::test::ScopedFieldTrials override_field_trials_; webrtc::RtcEventLogNullImpl event_log_; // Used in WebRtcVideoEngineVoiceTest, but defined here so it's properly @@ -3480,6 +3488,7 @@ TEST_F(WebRtcVideoChannelTest, EstimatesNtpStartTimeCorrectly) { // This timestamp is kInitialTimestamp (-1) + kFrameOffsetMs * 90, which // triggers a constant-overflow warning, hence we're calculating it explicitly // here. + fake_clock_.AdvanceTimeMicros(kFrameOffsetMs * rtc::kNumMicrosecsPerMillisec); video_frame.set_timestamp(kFrameOffsetMs * 90 - 1); video_frame.set_ntp_time_ms(kInitialNtpTimeMs + kFrameOffsetMs); stream->InjectFrame(video_frame);