From f938922c5c7021785769e9100ff3a5bc201196ff Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Wed, 21 Jan 2015 12:51:13 +0000 Subject: [PATCH] Simplify and guard access to WindowsRealTimeClock. Addresses data race in get_time() causing incorrect timer roll-over detection. This roll-over caused NTP timers to jump by 2^32 milliseconds affecting A/V sync and end-to-end delay calculations. BUG=4206 R=dvyukov@google.com, tommi@webrtc.org Review URL: https://webrtc-codereview.appspot.com/33959004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@8112 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/system_wrappers/source/clock.cc | 210 +++++++++++-------------- 1 file changed, 89 insertions(+), 121 deletions(-) diff --git a/webrtc/system_wrappers/source/clock.cc b/webrtc/system_wrappers/source/clock.cc index 33eb8561f..8e69652a8 100644 --- a/webrtc/system_wrappers/source/clock.cc +++ b/webrtc/system_wrappers/source/clock.cc @@ -12,14 +12,14 @@ #if defined(_WIN32) // Windows needs to be included before mmsystem.h -#include -#include +#include "webrtc/base/win32.h" #include #elif ((defined WEBRTC_LINUX) || (defined WEBRTC_MAC)) #include #include #endif +#include "webrtc/base/criticalsection.h" #include "webrtc/system_wrappers/interface/rw_lock_wrapper.h" #include "webrtc/system_wrappers/interface/tick_util.h" @@ -33,99 +33,6 @@ int64_t Clock::NtpToMs(uint32_t ntp_secs, uint32_t ntp_frac) { static_cast(ntp_frac_ms + 0.5); } -#if defined(_WIN32) - -struct reference_point { - FILETIME file_time; - LARGE_INTEGER counterMS; -}; - -struct WindowsHelpTimer { - volatile LONG _timeInMs; - volatile LONG _numWrapTimeInMs; - reference_point _ref_point; - - volatile LONG _sync_flag; -}; - -void Synchronize(WindowsHelpTimer* help_timer) { - const LONG start_value = 0; - const LONG new_value = 1; - const LONG synchronized_value = 2; - - LONG compare_flag = new_value; - while (help_timer->_sync_flag == start_value) { - const LONG new_value = 1; - compare_flag = InterlockedCompareExchange( - &help_timer->_sync_flag, new_value, start_value); - } - if (compare_flag != start_value) { - // This thread was not the one that incremented the sync flag. - // Block until synchronization finishes. - while (compare_flag != synchronized_value) { - ::Sleep(0); - } - return; - } - // Only the synchronizing thread gets here so this part can be - // considered single threaded. - - // set timer accuracy to 1 ms - timeBeginPeriod(1); - FILETIME ft0 = { 0, 0 }, - ft1 = { 0, 0 }; - // - // Spin waiting for a change in system time. Get the matching - // performance counter value for that time. - // - ::GetSystemTimeAsFileTime(&ft0); - do { - ::GetSystemTimeAsFileTime(&ft1); - - help_timer->_ref_point.counterMS.QuadPart = ::timeGetTime(); - ::Sleep(0); - } while ((ft0.dwHighDateTime == ft1.dwHighDateTime) && - (ft0.dwLowDateTime == ft1.dwLowDateTime)); - help_timer->_ref_point.file_time = ft1; - timeEndPeriod(1); -} - -void get_time(WindowsHelpTimer* help_timer, FILETIME& current_time) { - // we can't use query performance counter due to speed stepping - DWORD t = timeGetTime(); - // NOTE: we have a missmatch in sign between _timeInMs(LONG) and - // (DWORD) however we only use it here without +- etc - volatile LONG* timeInMsPtr = &help_timer->_timeInMs; - // Make sure that we only inc wrapper once. - DWORD old = InterlockedExchange(timeInMsPtr, t); - if(old > t) { - // wrap - help_timer->_numWrapTimeInMs++; - } - LARGE_INTEGER elapsedMS; - elapsedMS.HighPart = help_timer->_numWrapTimeInMs; - elapsedMS.LowPart = t; - - elapsedMS.QuadPart = elapsedMS.QuadPart - - help_timer->_ref_point.counterMS.QuadPart; - - // Translate to 100-nanoseconds intervals (FILETIME resolution) - // and add to reference FILETIME to get current FILETIME. - ULARGE_INTEGER filetime_ref_as_ul; - - filetime_ref_as_ul.HighPart = - help_timer->_ref_point.file_time.dwHighDateTime; - filetime_ref_as_ul.LowPart = - help_timer->_ref_point.file_time.dwLowDateTime; - filetime_ref_as_ul.QuadPart += - (ULONGLONG)((elapsedMS.QuadPart)*1000*10); - - // Copy to result - current_time.dwHighDateTime = filetime_ref_as_ul.HighPart; - current_time.dwLowDateTime = filetime_ref_as_ul.LowPart; -} -#endif - class RealTimeClock : public Clock { // Return a timestamp in milliseconds relative to some arbitrary source; the // source is fixed for this clock. @@ -178,14 +85,24 @@ class RealTimeClock : public Clock { }; #if defined(_WIN32) +// TODO(pbos): Consider modifying the implementation to synchronize itself +// against system time (update ref_point_, make it non-const) periodically to +// prevent clock drift. class WindowsRealTimeClock : public RealTimeClock { public: - WindowsRealTimeClock(WindowsHelpTimer* helpTimer) - : _helpTimer(helpTimer) {} + WindowsRealTimeClock() + : last_time_ms_(0), + num_timer_wraps_(0), + ref_point_(GetSystemReferencePoint()) {} virtual ~WindowsRealTimeClock() {} protected: + struct ReferencePoint { + FILETIME file_time; + LARGE_INTEGER counter_ms; + }; + virtual timeval CurrentTimeVal() const OVERRIDE { const uint64_t FILETIME_1970 = 0x019db1ded53e8000; @@ -195,7 +112,7 @@ class WindowsRealTimeClock : public RealTimeClock { // We can't use query performance counter since they can change depending on // speed stepping. - get_time(_helpTimer, StartTime); + GetTime(&StartTime); Time = (((uint64_t) StartTime.dwHighDateTime) << 32) + (uint64_t) StartTime.dwLowDateTime; @@ -208,7 +125,65 @@ class WindowsRealTimeClock : public RealTimeClock { return tv; } - WindowsHelpTimer* _helpTimer; + void GetTime(FILETIME* current_time) const { + DWORD t; + LARGE_INTEGER elapsed_ms; + { + rtc::CritScope lock(&crit_); + // time MUST be fetched inside the critical section to avoid non-monotonic + // last_time_ms_ values that'll register as incorrect wraparounds due to + // concurrent calls to GetTime. + t = timeGetTime(); + if (t < last_time_ms_) + num_timer_wraps_++; + last_time_ms_ = t; + elapsed_ms.HighPart = num_timer_wraps_; + } + elapsed_ms.LowPart = t; + elapsed_ms.QuadPart = elapsed_ms.QuadPart - ref_point_.counter_ms.QuadPart; + + // Translate to 100-nanoseconds intervals (FILETIME resolution) + // and add to reference FILETIME to get current FILETIME. + ULARGE_INTEGER filetime_ref_as_ul; + filetime_ref_as_ul.HighPart = ref_point_.file_time.dwHighDateTime; + filetime_ref_as_ul.LowPart = ref_point_.file_time.dwLowDateTime; + filetime_ref_as_ul.QuadPart += + static_cast((elapsed_ms.QuadPart) * 1000 * 10); + + // Copy to result + current_time->dwHighDateTime = filetime_ref_as_ul.HighPart; + current_time->dwLowDateTime = filetime_ref_as_ul.LowPart; + } + + static ReferencePoint GetSystemReferencePoint() { + ReferencePoint ref = {0}; + FILETIME ft0 = {0}; + FILETIME ft1 = {0}; + // Spin waiting for a change in system time. As soon as this change happens, + // get the matching call for timeGetTime() as soon as possible. This is + // assumed to be the most accurate offset that we can get between + // timeGetTime() and system time. + + // Set timer accuracy to 1 ms. + timeBeginPeriod(1); + GetSystemTimeAsFileTime(&ft0); + do { + GetSystemTimeAsFileTime(&ft1); + + ref.counter_ms.QuadPart = timeGetTime(); + Sleep(0); + } while ((ft0.dwHighDateTime == ft1.dwHighDateTime) && + (ft0.dwLowDateTime == ft1.dwLowDateTime)); + ref.file_time = ft1; + timeEndPeriod(1); + return ref; + } + + // mutable as time-accessing functions are const. + mutable rtc::CriticalSection crit_; + mutable DWORD last_time_ms_; + mutable LONG num_timer_wraps_; + const ReferencePoint ref_point_; }; #elif ((defined WEBRTC_LINUX) || (defined WEBRTC_MAC)) @@ -230,32 +205,25 @@ class UnixRealTimeClock : public RealTimeClock { }; #endif - #if defined(_WIN32) -// Keeps the global state for the Windows implementation of RtpRtcpClock. -// Note that this is a POD. Only PODs are allowed to have static storage -// duration according to the Google Style guide. -// -// Note that on Windows, GetSystemTimeAsFileTime has poorer (up to 15 ms) -// resolution than the media timers, hence the WindowsHelpTimer context -// object and Synchronize API to sync the two. -// -// We only sync up once, which means that on Windows, our realtime clock -// wont respond to system time/date changes without a program restart. -// TODO(henrike): We should probably call sync more often to catch -// drift and time changes for parity with other platforms. - -static WindowsHelpTimer *SyncGlobalHelpTimer() { - static WindowsHelpTimer global_help_timer = {0, 0, {{ 0, 0}, 0}, 0}; - Synchronize(&global_help_timer); - return &global_help_timer; -} +static WindowsRealTimeClock* volatile g_shared_clock = nullptr; #endif - Clock* Clock::GetRealTimeClock() { #if defined(_WIN32) - static WindowsRealTimeClock clock(SyncGlobalHelpTimer()); - return &clock; + // This read relies on volatile read being atomic-load-acquire. This is + // true in MSVC since at least 2005: + // "A read of a volatile object (volatile read) has Acquire semantics" + if (g_shared_clock != nullptr) + return g_shared_clock; + WindowsRealTimeClock* clock = new WindowsRealTimeClock; + if (InterlockedCompareExchangePointer( + reinterpret_cast(&g_shared_clock), clock, nullptr) != + nullptr) { + // g_shared_clock was assigned while we constructed/tried to assign our + // instance, delete our instance and use the existing one. + delete clock; + } + return g_shared_clock; #elif defined(WEBRTC_LINUX) || defined(WEBRTC_MAC) static UnixRealTimeClock clock; return &clock;