Bug 525401 - Make ready state transitions initiated by the decode thread more reliable by including the new state in the event. Fixes an ABA problem where we could play through without ever moving beyond HAVE_CURRENT_DATA. r=chris.double
authorMatthew Gregan <kinetik@flim.org>
Mon, 15 Mar 2010 12:46:38 +1300
changeset 39441 052a1c415457e7c725e1ca9ddb8cdd67c21da233
parent 39440 d856fe629ad2cc6785cfea9ad1e31c6e4dbbab34
child 39442 b884946383428a7d9b3a2aef0eea802ebaf5f5fa
push id12196
push usermgregan@mozilla.com
push dateMon, 15 Mar 2010 04:05:02 +0000
treeherdermozilla-central@052a1c415457 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschris
bugs525401
milestone1.9.3a3pre
Bug 525401 - Make ready state transitions initiated by the decode thread more reliable by including the new state in the event. Fixes an ABA problem where we could play through without ever moving beyond HAVE_CURRENT_DATA. r=chris.double
content/media/ogg/nsOggDecoder.cpp
content/media/ogg/nsOggDecoder.h
content/media/wave/nsWaveDecoder.cpp
content/media/wave/nsWaveDecoder.h
--- a/content/media/ogg/nsOggDecoder.cpp
+++ b/content/media/ogg/nsOggDecoder.cpp
@@ -366,16 +366,19 @@ public:
   // calling this.
   void SetVolume(float aVolume);
 
   // Clear the flag indicating that a playback position change event
   // is currently queued. This is called from the main thread and must
   // be called with the decode monitor held.
   void ClearPositionChangeFlag();
 
+  // Called by decoder and main thread.
+  nsHTMLMediaElement::NextFrameStatus GetNextFrameStatus();
+
   // Must be called with the decode monitor held. Can be called by main
   // thread.
   PRBool HaveNextFrameData() const {
     return !mDecodedFrames.IsEmpty() &&
       (mDecodedFrames.Peek()->mDecodedFrameTime > mCurrentFrameTime ||
        mDecodedFrames.GetCount() > 1);
   }
 
@@ -402,16 +405,37 @@ protected:
 
   // Convert the OggPlay frame information into a format used by Gecko
   // (RGB for video, float for sound, etc).The decoder monitor must be
   // acquired in the scope of calls to these functions. They must be
   // called only when the current state > DECODING_METADATA.
   void HandleVideoData(FrameData* aFrame, int aTrackNum, OggPlayDataHeader* aVideoHeader);
   void HandleAudioData(FrameData* aFrame, OggPlayAudioData* aAudioData, int aSize);
 
+  void UpdateReadyState() {
+    PR_ASSERT_CURRENT_THREAD_IN_MONITOR(mDecoder->GetMonitor());
+
+    nsCOMPtr<nsIRunnable> event;
+    switch (GetNextFrameStatus()) {
+      case nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE_BUFFERING:
+        event = NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, NextFrameUnavailableBuffering);
+        break;
+      case nsHTMLMediaElement::NEXT_FRAME_AVAILABLE:
+        event = NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, NextFrameAvailable);
+        break;
+      case nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE:
+        event = NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, NextFrameUnavailable);
+        break;
+      default:
+        PR_NOT_REACHED("unhandled frame state");
+    }
+
+    NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+  }
+
   // These methods can only be called on the decoding thread.
   void LoadOggHeaders(nsChannelReader* aReader);
 
   // Initializes and opens the audio stream. Called from the decode
   // thread only. Must be called with the decode monitor held.
   void OpenAudioStream();
 
   // Closes and releases resources used by the audio stream. Called
@@ -1144,36 +1168,46 @@ void nsOggDecodeStateMachine::UpdatePlay
   }
 }
 
 void nsOggDecodeStateMachine::QueueDecodedFrames()
 {
   PR_ASSERT_CURRENT_THREAD_IN_MONITOR(mDecoder->GetMonitor());
   FrameData* frame;
   while (!mDecodedFrames.IsFull() && (frame = NextFrame())) {
-    if (mDecodedFrames.GetCount() < 2) {
+    PRUint32 oldFrameCount = mDecodedFrames.GetCount();
+    mDecodedFrames.Push(frame);
+    if (oldFrameCount < 2) {
       // Transitioning from 0 to 1 frames or from 1 to 2 frames could
       // affect HaveNextFrameData and hence what UpdateReadyStateForData does.
       // This could change us from HAVE_CURRENT_DATA to HAVE_FUTURE_DATA
       // (or even HAVE_ENOUGH_DATA), so we'd better trigger an
-      // UpdateReadyStateForData.
-      nsCOMPtr<nsIRunnable> event = 
-        NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, UpdateReadyStateForData);
-      NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+      // update to the ready state.
+      UpdateReadyState();
     }
-    mDecodedFrames.Push(frame);
   }
 }
 
 void nsOggDecodeStateMachine::ClearPositionChangeFlag()
 {
   PR_ASSERT_CURRENT_THREAD_IN_MONITOR(mDecoder->GetMonitor());
   mPositionChangeQueued = PR_FALSE;
 }
 
+nsHTMLMediaElement::NextFrameStatus nsOggDecodeStateMachine::GetNextFrameStatus()
+{
+  nsAutoMonitor mon(mDecoder->GetMonitor());
+  if (IsBuffering() || IsSeeking()) {
+    return nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE_BUFFERING;
+  } else if (HaveNextFrameData()) {
+    return nsHTMLMediaElement::NEXT_FRAME_AVAILABLE;
+  }
+  return nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE;
+}
+
 void nsOggDecodeStateMachine::SetVolume(float volume)
 {
   PR_ASSERT_CURRENT_THREAD_IN_MONITOR(mDecoder->GetMonitor());
   if (mAudioStream) {
     mAudioStream->SetVolume(volume);
   }
 
   mVolume = volume;
@@ -1549,19 +1583,17 @@ nsresult nsOggDecodeStateMachine::Run()
           // We can't just directly send an asynchronous runnable that
           // eventually fires the "waiting" event. The problem is that
           // there might be pending main-thread events, such as "data
           // received" notifications, that mean we're not actually still
           // buffering by the time this runnable executes. So instead
           // we just trigger UpdateReadyStateForData; when it runs, it
           // will check the current state and decide whether to tell
           // the element we're buffering or not.
-          nsCOMPtr<nsIRunnable> event = 
-            NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, UpdateReadyStateForData);
-          NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+          UpdateReadyState();
 
           mBufferingStart = TimeStamp::Now();
           PRPackedBool reliable;
           double playbackRate = mDecoder->ComputePlaybackRate(&reliable);
           mBufferingEndOffset = mDecoder->mDecoderPosition +
             BUFFERING_RATE(playbackRate) * BUFFERING_WAIT;
           mState = DECODER_STATE_BUFFERING;
           if (mPlaying) {
@@ -1694,19 +1726,17 @@ nsresult nsOggDecodeStateMachine::Run()
           LOG(PR_LOG_DEBUG, ("%p Changed state from BUFFERING to DECODING", mDecoder));
           mState = DECODER_STATE_DECODING;
         }
 
         if (mState != DECODER_STATE_BUFFERING) {
           mBufferExhausted = PR_FALSE;
           // Notify to allow blocked decoder thread to continue
           mon.NotifyAll();
-          nsCOMPtr<nsIRunnable> event = 
-            NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, UpdateReadyStateForData);
-          NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+          UpdateReadyState();
           if (mDecoder->GetState() == nsOggDecoder::PLAY_STATE_PLAYING) {
             if (!mPlaying) {
               ResumePlayback();
             }
           }
         }
 
         break;
@@ -2385,33 +2415,51 @@ void nsOggDecoder::NotifyDownloadEnded(n
 void nsOggDecoder::NotifyBytesConsumed(PRInt64 aBytes)
 {
   nsAutoMonitor mon(mMonitor);
   if (!mIgnoreProgressData) {
     mDecoderPosition += aBytes;
   }
 }
 
-void nsOggDecoder::UpdateReadyStateForData()
+void nsOggDecoder::NextFrameUnavailableBuffering()
 {
+  NS_ASSERTION(NS_IsMainThread(), "Should be called on main thread");
+  if (!mElement || mShuttingDown || !mDecodeStateMachine)
+    return;
+
+  mElement->UpdateReadyStateForData(nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE_BUFFERING);
+}
+
+void nsOggDecoder::NextFrameAvailable()
+{
+  NS_ASSERTION(NS_IsMainThread(), "Should be called on main thread");
   if (!mElement || mShuttingDown || !mDecodeStateMachine)
     return;
 
-  nsHTMLMediaElement::NextFrameStatus frameStatus;
-  {
-    nsAutoMonitor mon(mMonitor);
-    if (mDecodeStateMachine->IsBuffering() ||
-        mDecodeStateMachine->IsSeeking()) {
-      frameStatus = nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE_BUFFERING;
-    } else if (mDecodeStateMachine->HaveNextFrameData()) {
-      frameStatus = nsHTMLMediaElement::NEXT_FRAME_AVAILABLE;
-    } else {
-      frameStatus = nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE;
-    }
-  }
+  mElement->UpdateReadyStateForData(nsHTMLMediaElement::NEXT_FRAME_AVAILABLE);
+}
+
+void nsOggDecoder::NextFrameUnavailable()
+{
+  NS_ASSERTION(NS_IsMainThread(), "Should be called on main thread");
+  if (!mElement || mShuttingDown || !mDecodeStateMachine)
+    return;
+
+  mElement->UpdateReadyStateForData(nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE);
+}
+
+void nsOggDecoder::UpdateReadyStateForData()
+{
+  NS_ASSERTION(NS_IsMainThread(), "Should be called on main thread");
+  if (!mElement || mShuttingDown || !mDecodeStateMachine)
+    return;
+
+  nsHTMLMediaElement::NextFrameStatus frameStatus =
+    mDecodeStateMachine->GetNextFrameStatus();
   mElement->UpdateReadyStateForData(frameStatus);
 }
 
 void nsOggDecoder::SeekingStopped()
 {
   if (mShuttingDown)
     return;
 
--- a/content/media/ogg/nsOggDecoder.h
+++ b/content/media/ogg/nsOggDecoder.h
@@ -454,16 +454,22 @@ protected:
   // thread.
   void SeekingStarted();
 
   // Called when the backend has changed the current playback
   // position. It dispatches a timeupdate event and invalidates the frame.
   // This must be called on the main thread only.
   void PlaybackPositionChanged();
 
+  // Calls mElement->UpdateReadyStateForData, telling it which state we have
+  // entered.  Main thread only.
+  void NextFrameUnavailableBuffering();
+  void NextFrameAvailable();
+  void NextFrameUnavailable();
+
   // Calls mElement->UpdateReadyStateForData, telling it whether we have
   // data for the next frame and if we're buffering. Main thread only.
   void UpdateReadyStateForData();
 
   // Find the end of the cached data starting at the current decoder
   // position.
   PRInt64 GetDownloadPosition();
 
--- a/content/media/wave/nsWaveDecoder.cpp
+++ b/content/media/wave/nsWaveDecoder.cpp
@@ -156,33 +156,54 @@ public:
   NS_IMETHOD Run();
 
   // Called by the decoder, on the main thread.
   nsMediaDecoder::Statistics GetStatistics();
 
   // Called on the decoder thread
   void NotifyBytesConsumed(PRInt64 aBytes);
 
-  // Called by the main thread only
+  // Called by decoder and main thread.
   nsHTMLMediaElement::NextFrameStatus GetNextFrameStatus();
 
   // Clear the flag indicating that a playback position change event is
   // currently queued and return the current time. This is called from the
   // main thread.
   float GetTimeForPositionChange();
 
 private:
   // Returns PR_TRUE if we're in shutdown state. Threadsafe.
   PRBool IsShutdown();
 
   // Reads from the media stream. Returns PR_FALSE on failure or EOF.  If
   // aBytesRead is non-null, the number of bytes read will be returned via
   // this.
   PRBool ReadAll(char* aBuf, PRInt64 aSize, PRInt64* aBytesRead);
 
+  void UpdateReadyState() {
+    PR_ASSERT_CURRENT_THREAD_IN_MONITOR(mMonitor);
+
+    nsCOMPtr<nsIRunnable> event;
+    switch (GetNextFrameStatus()) {
+      case nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE_BUFFERING:
+        event = NS_NEW_RUNNABLE_METHOD(nsWaveDecoder, mDecoder, NextFrameUnavailableBuffering);
+        break;
+      case nsHTMLMediaElement::NEXT_FRAME_AVAILABLE:
+        event = NS_NEW_RUNNABLE_METHOD(nsWaveDecoder, mDecoder, NextFrameAvailable);
+        break;
+      case nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE:
+        event = NS_NEW_RUNNABLE_METHOD(nsWaveDecoder, mDecoder, NextFrameUnavailable);
+        break;
+      default:
+        PR_NOT_REACHED("unhandled frame state");
+    }
+
+    NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+  }
+
   // Change the current state and wake the playback thread if it is waiting
   // on mMonitor.  Used by public member functions called from both threads,
   // so must hold mMonitor.  Threadsafe.
   void ChangeState(State aState);
 
   // Create and initialize audio stream using current audio parameters.
   void OpenAudioStream();
 
@@ -524,19 +545,17 @@ nsWaveStateMachine::Run()
           !mStream->IsSuspendedByCache()) {
         LOG(PR_LOG_DEBUG,
             ("In buffering: buffering data until %d bytes available or %f seconds\n",
              PRUint32(mBufferingEndOffset - mStream->GetCachedDataEnd(mPlaybackPosition)),
              (mBufferingWait - (now - mBufferingStart)).ToSeconds()));
         monitor.Wait(PR_MillisecondsToInterval(1000));
       } else {
         ChangeState(mNextState);
-        nsCOMPtr<nsIRunnable> event =
-          NS_NEW_RUNNABLE_METHOD(nsWaveDecoder, mDecoder, UpdateReadyStateForData);
-        NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+        UpdateReadyState();
       }
 
       break;
     }
 
     case STATE_PLAYING: {
       if (!mAudioStream) {
         OpenAudioStream();
@@ -585,19 +604,17 @@ nsWaveStateMachine::Run()
             !mStream->IsSuspendedByCache()) {
           mBufferingStart = now;
           mBufferingEndOffset = mPlaybackPosition +
             TimeToBytes(mBufferingWait.ToSeconds());
           mBufferingEndOffset = PR_MAX(mPlaybackPosition + len, mBufferingEndOffset);
           mNextState = mState;
           ChangeState(STATE_BUFFERING);
 
-          nsCOMPtr<nsIRunnable> event =
-            NS_NEW_RUNNABLE_METHOD(nsWaveDecoder, mDecoder, UpdateReadyStateForData);
-          NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+          UpdateReadyState();
           break;
         }
 
         if (len > 0) {
           nsAutoArrayPtr<char> buf(new char[size_t(len)]);
           PRInt64 got = 0;
 
           monitor.Exit();
@@ -1481,18 +1498,53 @@ nsWaveDecoder::Observe(nsISupports* aSub
 {
   if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
     Shutdown();
   }
   return NS_OK;
 }
 
 void
+nsWaveDecoder::NextFrameUnavailableBuffering()
+{
+  NS_ASSERTION(NS_IsMainThread(), "Should be called on main thread");
+  if (!mElement || mShuttingDown || !mPlaybackStateMachine)
+    return;
+
+  mElement->UpdateReadyStateForData(nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE_BUFFERING);
+}
+
+void
+nsWaveDecoder::NextFrameAvailable()
+{
+  NS_ASSERTION(NS_IsMainThread(), "Should be called on main thread");
+  if (!mElement || mShuttingDown || !mPlaybackStateMachine)
+    return;
+
+  if (!mMetadataLoadedReported) {
+    mElement->UpdateReadyStateForData(nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE);
+  } else {
+    mElement->UpdateReadyStateForData(nsHTMLMediaElement::NEXT_FRAME_AVAILABLE);
+  }
+}
+
+void
+nsWaveDecoder::NextFrameUnavailable()
+{
+  NS_ASSERTION(NS_IsMainThread(), "Should be called on main thread");
+  if (!mElement || mShuttingDown || !mPlaybackStateMachine)
+    return;
+
+  mElement->UpdateReadyStateForData(nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE);
+}
+
+void
 nsWaveDecoder::UpdateReadyStateForData()
 {
+  NS_ASSERTION(NS_IsMainThread(), "Should be called on main thread");
   if (!mElement || mShuttingDown || !mPlaybackStateMachine)
     return;
 
   nsHTMLMediaElement::NextFrameStatus frameStatus =
     mPlaybackStateMachine->GetNextFrameStatus();
   if (frameStatus == nsHTMLMediaElement::NEXT_FRAME_AVAILABLE &&
       !mMetadataLoadedReported) {
     frameStatus = nsHTMLMediaElement::NEXT_FRAME_UNAVAILABLE;
--- a/content/media/wave/nsWaveDecoder.h
+++ b/content/media/wave/nsWaveDecoder.h
@@ -216,16 +216,22 @@ class nsWaveDecoder : public nsMediaDeco
   // thread only.
   virtual void Suspend();
 
   // Resume any media downloads that have been suspended. Called by the
   // media element when it is restored from the bfcache. Call on the
   // main thread only.
   virtual void Resume();
 
+  // Calls mElement->UpdateReadyStateForData, telling it which state we have
+  // entered.  Main thread only.
+  void NextFrameUnavailableBuffering();
+  void NextFrameAvailable();
+  void NextFrameUnavailable();
+
   // Change the element's ready state as necessary. Main thread only.
   void UpdateReadyStateForData();
 
   // Tells mStream to put all loads in the background.
   virtual void MoveLoadsToBackground();
 
   // Called asynchronously to shut down the decoder
   void Stop();