Bug 1443942 - Move code to toggle high res timers into VideoSink. r=jya
authorChris Pearce <cpearce@mozilla.com>
Fri, 06 Apr 2018 13:33:28 +1200
changeset 779719 2a39d6a9a949dac9c9cba0a9dfc1bd93de88c79e
parent 779718 8b3f66ef0cc427ba2afca030b31194fb5b7aa95b
child 779720 72144da2637d2bd2f0695010628c6db651482e23
push id105836
push userbmo:ato@sny.no
push dateTue, 10 Apr 2018 12:07:22 +0000
reviewersjya
bugs1443942
milestone61.0a1
Bug 1443942 - Move code to toggle high res timers into VideoSink. r=jya We have code in the MDSM to toggle on high resolution timers on Windows when we start/stop playing because the VideoSink relies on being awoken by timers to update the set of current frames in the compositor's queue, and on Windows 7 we end up dropping frames due to the timer lag without this. We assert in the MDSM's destructor that we've turned off high res timers (as they cause needless battery drain, so we only want them on when we need them), and the new test_mediarecorder_principals is hitting that assert on Windows. I think we're missing turning them off when we create a new VideoSink for outputting to the MSG. That affects the value returned by MediaDecoderStateMachine->mVideoSink->IsPlaying(), which is what we use to decide whether we should enable high resolution timers. We track whether we've enabled high res timers in MDSM::mHiResTimersRequested, and that gets out of sync with IsPlaying() when we re-create the MediaSink. Rather than trying to handle all the permutations of places where we need to turn off high resolution timers in the MDSM, we're better to move the code to toggle high res timers into the VideoSink, as that's actually where we need to be sure that we have high resolution timers enabled anyway. It's the VideoSink after all that is relying on timers for frame update, not the MDSM. Also remove the media.hi-res-timers.enabled pref, as we haven't needed it. MozReview-Commit-ID: 9dNxcYxPDZH
dom/media/MediaDecoderStateMachine.cpp
dom/media/MediaDecoderStateMachine.h
dom/media/mediasink/VideoSink.cpp
dom/media/mediasink/VideoSink.h
--- a/dom/media/MediaDecoderStateMachine.cpp
+++ b/dom/media/MediaDecoderStateMachine.cpp
@@ -1,33 +1,26 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim:set ts=2 sw=2 sts=2 et cindent: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-#ifdef XP_WIN
-// Include Windows headers required for enabling high precision timers.
-#include "windows.h"
-#include "mmsystem.h"
-#endif
-
 #include <algorithm>
 #include <stdint.h>
 
 #include "mediasink/AudioSink.h"
 #include "mediasink/AudioSinkWrapper.h"
 #include "mediasink/DecodedStream.h"
 #include "mediasink/OutputStreamManager.h"
 #include "mediasink/VideoSink.h"
 #include "mozilla/IndexSequence.h"
 #include "mozilla/Logging.h"
 #include "mozilla/MathAlgorithms.h"
 #include "mozilla/NotNull.h"
-#include "mozilla/Preferences.h"
 #include "mozilla/SharedThreadPool.h"
 #include "mozilla/Sprintf.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/TaskQueue.h"
 #include "mozilla/Tuple.h"
 #include "nsIMemoryReporter.h"
 #include "nsPrintfCString.h"
 #include "nsTArray.h"
@@ -2700,19 +2693,16 @@ MediaDecoderStateMachine::MediaDecoderSt
   INIT_MIRROR(mVolume, 1.0),
   INIT_MIRROR(mPreservesPitch, true),
   INIT_MIRROR(mLooping, false),
   INIT_MIRROR(mSameOriginMedia, false),
   INIT_MIRROR(mMediaPrincipalHandle, PRINCIPAL_HANDLE_NONE),
   INIT_CANONICAL(mDuration, NullableTimeUnit()),
   INIT_CANONICAL(mCurrentPosition, TimeUnit::Zero()),
   INIT_CANONICAL(mIsAudioDataAudible, false)
-#ifdef XP_WIN
-  , mShouldUseHiResTimers(Preferences::GetBool("media.hi-res-timers.enabled", true))
-#endif
 {
   MOZ_COUNT_CTOR(MediaDecoderStateMachine);
   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
 
   InitVideoQueuePrefs();
 
   DDLINKCHILD("reader", aReader);
 }
@@ -2720,20 +2710,16 @@ MediaDecoderStateMachine::MediaDecoderSt
 #undef INIT_WATCHABLE
 #undef INIT_MIRROR
 #undef INIT_CANONICAL
 
 MediaDecoderStateMachine::~MediaDecoderStateMachine()
 {
   MOZ_ASSERT(NS_IsMainThread(), "Should be on main thread.");
   MOZ_COUNT_DTOR(MediaDecoderStateMachine);
-
-#ifdef XP_WIN
-  MOZ_ASSERT(!mHiResTimersRequested);
-#endif
 }
 
 void
 MediaDecoderStateMachine::InitializationTask(MediaDecoder* aDecoder)
 {
   MOZ_ASSERT(OnTaskQueue());
 
   // Connect mirrors.
@@ -2924,22 +2910,16 @@ MediaDecoderStateMachine::StopPlayback()
   MOZ_ASSERT(OnTaskQueue());
   LOG("StopPlayback()");
 
   if (IsPlaying()) {
     mOnPlaybackEvent.Notify(MediaPlaybackEvent{
       MediaPlaybackEvent::PlaybackStopped, mPlaybackOffset });
     mMediaSink->SetPlaying(false);
     MOZ_ASSERT(!IsPlaying());
-#ifdef XP_WIN
-    if (mHiResTimersRequested) {
-      mHiResTimersRequested = false;
-      timeEndPeriod(1);
-    }
-#endif
   }
 }
 
 void MediaDecoderStateMachine::MaybeStartPlayback()
 {
   MOZ_ASSERT(OnTaskQueue());
   // Should try to start playback only after decoding first frames.
   MOZ_ASSERT(mSentFirstFrameLoadedEvent);
@@ -2952,30 +2932,16 @@ void MediaDecoderStateMachine::MaybeStar
   if (mPlayState != MediaDecoder::PLAY_STATE_PLAYING) {
     LOG("Not starting playback [mPlayState=%d]", mPlayState.Ref());
     return;
   }
 
   LOG("MaybeStartPlayback() starting playback");
   StartMediaSink();
 
-#ifdef XP_WIN
-  if (!mHiResTimersRequested && mShouldUseHiResTimers) {
-    mHiResTimersRequested = true;
-    // Ensure high precision timers are enabled on Windows, otherwise the state
-    // machine isn't woken up at reliable intervals to set the next frame, and we
-    // drop frames while painting. Note that each call must be matched by a
-    // corresponding timeEndPeriod() call. Enabling high precision timers causes
-    // the CPU to wake up more frequently on Windows 7 and earlier, which causes
-    // more CPU load and battery use. So we only enable high precision timers
-    // when we're actually playing.
-    timeBeginPeriod(1);
-  }
-#endif
-
   if (!IsPlaying()) {
     mMediaSink->SetPlaying(true);
     MOZ_ASSERT(IsPlaying());
   }
 
   mOnPlaybackEvent.Notify(
     MediaPlaybackEvent{ MediaPlaybackEvent::PlaybackStarted, mPlaybackOffset });
 }
--- a/dom/media/MediaDecoderStateMachine.h
+++ b/dom/media/MediaDecoderStateMachine.h
@@ -746,24 +746,13 @@ public:
   AbstractCanonical<media::TimeUnit>* CanonicalCurrentPosition()
   {
     return &mCurrentPosition;
   }
   AbstractCanonical<bool>* CanonicalIsAudioDataAudible()
   {
     return &mIsAudioDataAudible;
   }
-
-#ifdef XP_WIN
-  // Whether we've called timeBeginPeriod(1) to request high resolution
-  // timers. We request high resolution timers when playback starts, and
-  // turn them off when playback is paused. Enabling high resolution
-  // timers can cause higher CPU usage and battery drain on Windows 7.
-  bool mHiResTimersRequested = false;
-  // Whether we should enable high resolution timers. This is initialized at
-  // MDSM construction, and mirrors the value of media.hi-res-timers.enabled.
-  const bool mShouldUseHiResTimers;
-#endif
 };
 
 } // namespace mozilla
 
 #endif
--- a/dom/media/mediasink/VideoSink.cpp
+++ b/dom/media/mediasink/VideoSink.cpp
@@ -1,14 +1,20 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+#ifdef XP_WIN
+// Include Windows headers required for enabling high precision timers.
+#include "windows.h"
+#include "mmsystem.h"
+#endif
+
 #include "MediaQueue.h"
 #include "VideoSink.h"
 #include "MediaPrefs.h"
 #include "VideoUtils.h"
 
 #include "mozilla/IntegerPrintfMacros.h"
 
 namespace mozilla {
@@ -40,22 +46,29 @@ VideoSink::VideoSink(AbstractThread* aTh
   , mVideoQueue(aVideoQueue)
   , mContainer(aContainer)
   , mProducerID(ImageContainer::AllocateProducerID())
   , mFrameStats(aFrameStats)
   , mHasVideo(false)
   , mUpdateScheduler(aThread)
   , mVideoQueueSendToCompositorSize(aVQueueSentToCompositerSize)
   , mMinVideoQueueSize(MediaPrefs::RuinAvSync() ? 1 : 0)
+#ifdef XP_WIN
+  , mHiResTimersRequested(false)
+#endif
+
 {
   MOZ_ASSERT(mAudioSink, "AudioSink should exist.");
 }
 
 VideoSink::~VideoSink()
 {
+#ifdef XP_WIN
+  MOZ_ASSERT(!mHiResTimersRequested);
+#endif
 }
 
 const MediaSink::PlaybackParams&
 VideoSink::GetPlaybackParams() const
 {
   AssertOwnerThread();
 
   return mAudioSink->GetPlaybackParams();
@@ -133,16 +146,40 @@ void
 VideoSink::SetPreservesPitch(bool aPreservesPitch)
 {
   AssertOwnerThread();
 
   mAudioSink->SetPreservesPitch(aPreservesPitch);
 }
 
 void
+VideoSink::EnsureHighResTimersOnOnlyIfPlaying()
+{
+#ifdef XP_WIN
+  const bool needed = IsPlaying();
+  if (needed == mHiResTimersRequested) {
+    return;
+  }
+  if (needed) {
+    // Ensure high precision timers are enabled on Windows, otherwise the VideoSink
+    // isn't woken up at reliable intervals to set the next frame, and we
+    // drop frames while painting. Note that each call must be matched by a
+    // corresponding timeEndPeriod() call. Enabling high precision timers causes
+    // the CPU to wake up more frequently on Windows 7 and earlier, which causes
+    // more CPU load and battery use. So we only enable high precision timers
+    // when we're actually playing.
+    timeBeginPeriod(1);
+  } else {
+    timeEndPeriod(1);
+  }
+  mHiResTimersRequested = needed;
+#endif
+}
+
+void
 VideoSink::SetPlaying(bool aPlaying)
 {
   AssertOwnerThread();
   VSINK_LOG_V(" playing (%d) -> (%d)", mAudioSink->IsPlaying(), aPlaying);
 
   if (!aPlaying) {
     // Reset any update timer if paused.
     mUpdateScheduler.Reset();
@@ -156,16 +193,18 @@ VideoSink::SetPlaying(bool aPlaying)
   mAudioSink->SetPlaying(aPlaying);
 
   if (mHasVideo && aPlaying) {
     // There's no thread in VideoSink for pulling video frames, need to trigger
     // rendering while becoming playing status. because the VideoQueue may be
     // full already.
     TryUpdateRenderedVideoFrames();
   }
+
+  EnsureHighResTimersOnOnlyIfPlaying();
 }
 
 void
 VideoSink::Start(const TimeUnit& aStartTime, const MediaInfo& aInfo)
 {
   AssertOwnerThread();
   VSINK_LOG("[%s]", __func__);
 
@@ -219,16 +258,18 @@ VideoSink::Stop()
   mUpdateScheduler.Reset();
   if (mHasVideo) {
     DisconnectListener();
     mVideoSinkEndRequest.DisconnectIfExists();
     mEndPromiseHolder.ResolveIfExists(true, __func__);
     mEndPromise = nullptr;
   }
   mVideoFrameEndTime = TimeUnit::Zero();
+
+  EnsureHighResTimersOnOnlyIfPlaying();
 }
 
 bool
 VideoSink::IsStarted() const
 {
   AssertOwnerThread();
 
   return mAudioSink->IsStarted();
--- a/dom/media/mediasink/VideoSink.h
+++ b/dom/media/mediasink/VideoSink.h
@@ -74,16 +74,18 @@ private:
   virtual ~VideoSink();
 
   // VideoQueue listener related.
   void OnVideoQueuePushed(RefPtr<VideoData>&& aSample);
   void OnVideoQueueFinished();
   void ConnectListener();
   void DisconnectListener();
 
+  void EnsureHighResTimersOnOnlyIfPlaying();
+
   // Sets VideoQueue images into the VideoFrameContainer. Called on the shared
   // state machine thread. The first aMaxFrames (at most) are set.
   // aClockTime and aClockTimeStamp are used as the baseline for deriving
   // timestamps for the frames; when omitted, aMaxFrames must be 1 and
   // a null timestamp is passed to the VideoFrameContainer.
   // If the VideoQueue is empty, this does nothing.
   void RenderVideoFrames(int32_t aMaxFrames, int64_t aClockTime = 0,
                          const TimeStamp& aClickTimeStamp = TimeStamp());
@@ -146,14 +148,23 @@ private:
   // Talos tests for the compositor require at least one frame in the
   // video queue so that the compositor has something to composit during
   // the talos test when the decode is stressed. We have a minimum size
   // on the video queue in order to facilitate this talos test.
   // Note: Normal playback should not have a queue size of more than 0,
   // otherwise A/V sync will be ruined! *Only* make this non-zero for
   // testing purposes.
   const uint32_t mMinVideoQueueSize;
+
+#ifdef XP_WIN
+  // Whether we've called timeBeginPeriod(1) to request high resolution
+  // timers. We request high resolution timers when playback starts, and
+  // turn them off when playback is paused. Enabling high resolution
+  // timers can cause higher CPU usage and battery drain on Windows 7,
+  // but reduces our frame drop rate.
+  bool mHiResTimersRequested;
+#endif
 };
 
 } // namespace media
 } // namespace mozilla
 
 #endif