Removes a global non POD instance from the RTP_RTCP module that was introduced in https://code.google.com/p/webrtc/source/detail?r=1076.

Review URL: https://webrtc-codereview.appspot.com/314001

git-svn-id: http://webrtc.googlecode.com/svn/trunk@1698 4adac7df-926f-26a2-2b94-8c16560cd09d

src/modules/rtp_rtcp/interface/rtp_rtcp_defines.h

@@ -251,15 +251,14 @@ protected:
 // timestamps in an RTP/RTCP module.
 class RtpRtcpClock {
 public:
+    virtual ~RtpRtcpClock() {}
+
     // Return a timestamp in milliseconds relative to some arbitrary
     // source; the source is fixed for this clock.
     virtual WebRtc_UWord32 GetTimeInMS() = 0;
 
     // Retrieve an NTP absolute timestamp.
     virtual void CurrentNTP(WebRtc_UWord32& secs, WebRtc_UWord32& frac) = 0;
-
-protected:
-    virtual ~RtpRtcpClock() {}
 };
 
 // RtpReceiveBitrateUpdate is used to signal changes in bitrate estimates for

src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc

@@ -37,8 +37,19 @@ namespace webrtc {
 const WebRtc_UWord16 kDefaultRtt = 200;
 
 RtpRtcp* RtpRtcp::CreateRtpRtcp(const WebRtc_Word32 id,
-                                const bool audio) {
+                                bool audio) {
-  return CreateRtpRtcp(id, audio, ModuleRTPUtility::GetSystemClock());
+  if(audio) {
+    WEBRTC_TRACE(kTraceModuleCall, kTraceRtpRtcp, id, "CreateRtpRtcp(audio)");
+  } else {
+    WEBRTC_TRACE(kTraceModuleCall, kTraceRtpRtcp, id, "CreateRtpRtcp(video)");
+  }
+  // ModuleRTPUtility::GetSystemClock() creates a new instance of a system
+  // clock implementation. The OwnsClock() function informs the module that
+  // it is responsible for deleting the instance.
+  ModuleRtpRtcpImpl* rtp_rtcp_instance = new ModuleRtpRtcpImpl(id,
+      audio, ModuleRTPUtility::GetSystemClock());
+  rtp_rtcp_instance->OwnsClock();
+  return rtp_rtcp_instance;
 }
 
 RtpRtcp* RtpRtcp::CreateRtpRtcp(const WebRtc_Word32 id,
@@ -76,6 +87,7 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const WebRtc_Word32 id,
   _rtpReceiver(id, audio, clock, this),
   _rtcpSender(id, audio, clock, this),
   _rtcpReceiver(id, clock, this),
+  _owns_clock(false),
   _clock(*clock),
   _id(id),
   _audio(audio),
@@ -83,7 +95,6 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const WebRtc_Word32 id,
   _lastProcessTime(clock->GetTimeInMS()),
   _lastBitrateProcessTime(clock->GetTimeInMS()),
   _lastPacketTimeoutProcessTime(clock->GetTimeInMS()),
-
   _packetOverHead(28), // IPV4 UDP
   _criticalSectionModulePtrs(CriticalSectionWrapper::CreateCriticalSection()),
   _criticalSectionModulePtrsFeedback(
@@ -156,6 +167,9 @@ ModuleRtpRtcpImpl::~ModuleRtpRtcpImpl() {
 
   delete _criticalSectionModulePtrs;
   delete _criticalSectionModulePtrsFeedback;
+  if (_owns_clock) {
+    delete &_clock;
+  }
 }
 
 WebRtc_Word32 ModuleRtpRtcpImpl::ChangeUniqueId(const WebRtc_Word32 id) {

src/modules/rtp_rtcp/source/rtp_rtcp_impl.h

@@ -535,6 +535,10 @@ public:
 
     void OnRequestSendReport();
 
+    // Following function is only called when constructing the object so no
+    // need to worry about data race.
+    void OwnsClock() { _owns_clock = true; }
+
 protected:
     void RegisterChildModule(RtpRtcp* module);
 
@@ -560,6 +564,7 @@ protected:
     RTCPSender                _rtcpSender;
     RTCPReceiver              _rtcpReceiver;
 
+    bool                      _owns_clock;
     RtpRtcpClock&             _clock;
 private:
     void SendKeyFrame();

src/modules/rtp_rtcp/source/rtp_utility.cc

@@ -50,99 +50,109 @@ namespace ModuleRTPUtility {
 
 #if defined(_WIN32)
 
-
+struct reference_point {
-class WindowsHelpTimer {
+  FILETIME      file_time;
-public:
+  LARGE_INTEGER counterMS;
-  struct reference_point {
-    FILETIME      file_time;
-    LARGE_INTEGER counterMS;
-  };
-
-  WindowsHelpTimer() {
-    // set timer accuracy to 1 ms
-    timeBeginPeriod(1);
-
-    _timeInMs = 0;
-    _numWrapTimeInMs = 0;
-    synchronize();
-  };
-
-  virtual ~WindowsHelpTimer() {
-    timeEndPeriod(1);
-  };
-
-  void get_time(FILETIME& current_time) {
-    // we can't use query performance counter due to speed stepping
-    DWORD t = timeGetTime();
-    // NOTE: we have a miss match in sign between _timeInMs(LONG) and t(DWORD)
-    // however we only use it here without +- etc
-    volatile LONG* timeInMsPtr = &_timeInMs;
-    DWORD old = InterlockedExchange(timeInMsPtr, t); // make sure that we only
-    // inc wrapper once
-    if (old > t) {
-      // wrap
-      _numWrapTimeInMs++;
-    }
-    LARGE_INTEGER elapsedMS;
-    elapsedMS.HighPart = _numWrapTimeInMs;
-    elapsedMS.LowPart = t;
-
-    elapsedMS.QuadPart = elapsedMS.QuadPart - _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 = _ref_point.file_time.dwHighDateTime;
-    filetime_ref_as_ul.LowPart = _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;
-  };
-
-private:
-  void synchronize() {
-    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);
-      _ref_point.counterMS.QuadPart = ::timeGetTime();
-      ::Sleep(0);
-    } while ((ft0.dwHighDateTime == ft1.dwHighDateTime) &&
-             (ft0.dwLowDateTime == ft1.dwLowDateTime));
-
-    _ref_point.file_time = ft1;
-  }
-  // this needs to be long due to Windows, not an issue due to its usage
-  volatile LONG           _timeInMs;
-  volatile WebRtc_UWord32 _numWrapTimeInMs;
-  reference_point         _ref_point;
 };
 
-// A clock reading times from the Windows API.
+struct WindowsHelpTimer {
-class WindowsSystemClock : public RtpRtcpClock {
+  volatile LONG _timeInMs;
-public:
+  volatile LONG _numWrapTimeInMs;
-  WindowsSystemClock()
+  reference_point _ref_point;
-    : _helpTimer() {}
 
-  virtual ~WindowsSystemClock() {}
+  volatile LONG _sync_flag;
+};
 
-  virtual WebRtc_UWord32 GetTimeInMS();
+void Synchronize(WindowsHelpTimer* help_timer) {
+  const LONG start_value = 0;
+  const LONG new_value = 1;
+  const LONG synchronized_value = 2;
 
-  virtual void CurrentNTP(WebRtc_UWord32& secs, WebRtc_UWord32& frac);
+  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.
 
-private:
+  // set timer accuracy to 1 ms
-  WindowsHelpTimer _helpTimer;
+  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;
+}
+
+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;
+  }
+
+  // A clock reading times from the Windows API.
+  class WindowsSystemClock : public RtpRtcpClock {
+  public:
+    WindowsSystemClock(WindowsHelpTimer* helpTimer)
+      : _helpTimer(helpTimer) {}
+
+    virtual ~WindowsSystemClock() {}
+
+    virtual WebRtc_UWord32 GetTimeInMS();
+
+    virtual void CurrentNTP(WebRtc_UWord32& secs, WebRtc_UWord32& frac);
+
+  private:
+    WindowsHelpTimer* _helpTimer;
 };
 
 #elif defined(WEBRTC_LINUX) || defined(WEBRTC_MAC)
@@ -174,9 +184,9 @@ void WindowsSystemClock::CurrentNTP(WebRtc_UWord32& secs,
   WebRtc_UWord64 Time;
   struct timeval tv;
 
-  // we can't use query performance counter since they can change depending on
+  // We can't use query performance counter since they can change depending on
   // speed steping
-  _helpTimer.get_time(StartTime);
+  get_time(_helpTimer, StartTime);
 
   Time = (((WebRtc_UWord64) StartTime.dwHighDateTime) << 32) +
          (WebRtc_UWord64) StartTime.dwLowDateTime;
@@ -238,21 +248,34 @@ void UnixSystemClock::CurrentNTP(WebRtc_UWord32& secs, WebRtc_UWord32& frac) {
 }
 #endif
 
-RtpRtcpClock* GetSystemClock() {
-  // TODO(hellner): violates the style guide (non-POD static instance).
 #if defined(_WIN32)
-  static WindowsSystemClock system_clock;
+// Keeps the global state for the Windows implementation of RtpRtcpClock.
-#elif defined(WEBRTC_LINUX) || defined(WEBRTC_MAC)
+// Note that this is a POD. Only PODs are allowed to have static storage
-  static UnixSystemClock system_clock;
+// duration according to the Google Style guide.
+static WindowsHelpTimer global_help_timer = {0, 0, {{ 0, 0}, 0}, 0};
+#endif
+
+RtpRtcpClock* GetSystemClock() {
+#if defined(_WIN32)
+  return new WindowsSystemClock(&global_help_timer);
+#elif defined(WEBRTC_LINUX) || defined(WEBRTC_MAC)
+  return new UnixSystemClock();
+#else
+  return NULL;
 #endif
-  return &system_clock;
 }
 
 WebRtc_UWord32 GetCurrentRTP(RtpRtcpClock* clock, WebRtc_UWord32 freq) {
-  if (clock == NULL)
+  const bool use_global_clock = (clock == NULL);
-    clock = GetSystemClock();
+  RtpRtcpClock* local_clock = clock;
+  if (use_global_clock) {
+    local_clock = GetSystemClock();
+  }
   WebRtc_UWord32 secs = 0, frac = 0;
-  clock->CurrentNTP(secs, frac);
+  local_clock->CurrentNTP(secs, frac);
+  if (use_global_clock) {
+    delete local_clock;
+  }
   return ConvertNTPTimeToRTP(secs, frac, freq);
 }
 

src/modules/rtp_rtcp/source/rtp_utility.h

@@ -65,6 +65,10 @@ namespace ModuleRTPUtility
     // system. The returned instances are guaranteed to read the same
     // times; in particular, they return relative times relative to
     // the same base.
+    // Note that even though the instances returned by this function
+    // read the same times a new object is created every time this
+    // API is called. The ownership of this object belongs to the
+    // caller.
     RtpRtcpClock* GetSystemClock();
 
     // Return the current RTP timestamp from the NTP timestamp