Bug 757707 - Avoid holding nsBufferedAudioStream locks over calls into cubeb. r=roc

content/media/nsAudioStream.cpp

@@ -1015,8 +1015,10 @@ nsBufferedAudioStream::Init(PRInt32 aNumChannels, PRInt32 aRate, SampleFormat aF
 void
 nsBufferedAudioStream::Shutdown()
 {
+  if (mState == STARTED) {
+    Pause();
+  }
   if (mCubebStream) {
-    cubeb_stream_stop(mCubebStream);
     mCubebStream.reset();
   }
 }
@@ -1041,17 +1043,22 @@ nsBufferedAudioStream::Write(const void* aBuf, PRUint32 aFrames)
     src += available;
     bytesToCopy -= available;
 
-    if (mState != STARTED && cubeb_stream_start(mCubebStream) == CUBEB_OK) {
-      mState = STARTED;
-    }
-
-    if (mState == STARTED && bytesToCopy > 0) {
-      mon.Wait();
+    if (mState != STARTED) {
+      int r;
+      {
+        MonitorAutoUnlock mon(mMonitor);
+        r = cubeb_stream_start(mCubebStream);
+      }
+      mState = r == CUBEB_OK ? STARTED : ERRORED;
     }
 
     if (mState != STARTED) {
       return NS_ERROR_FAILURE;
     }
+
+    if (bytesToCopy > 0) {
+      mon.Wait();
+    }
   }
 
   return NS_OK;
@@ -1065,7 +1072,8 @@ nsBufferedAudioStream::Available()
   return mBuffer.Available() / mBytesPerFrame;
 }
 
-PRInt32 nsBufferedAudioStream::GetMinWriteSize()
+PRInt32
+nsBufferedAudioStream::GetMinWriteSize()
 {
   return 1;
 }
@@ -1099,7 +1107,12 @@ nsBufferedAudioStream::Pause()
     return;
   }
 
-  if (cubeb_stream_stop(mCubebStream) == CUBEB_OK) {
+  int r;
+  {
+    MonitorAutoUnlock mon(mMonitor);
+    r = cubeb_stream_stop(mCubebStream);
+  }
+  if (mState != ERRORED && r == CUBEB_OK) {
     mState = STOPPED;
   }
 }
@@ -1112,12 +1125,18 @@ nsBufferedAudioStream::Resume()
     return;
   }
 
-  if (cubeb_stream_start(mCubebStream) == CUBEB_OK) {
+  int r;
+  {
+    MonitorAutoUnlock mon(mMonitor);
+    r = cubeb_stream_start(mCubebStream);
+  }
+  if (mState != ERRORED && r == CUBEB_OK) {
     mState = STARTED;
   }
 }
 
-PRInt64 nsBufferedAudioStream::GetPosition()
+PRInt64
+nsBufferedAudioStream::GetPosition()
 {
   MonitorAutoLock mon(mMonitor);
   PRInt64 frames = GetPositionInFramesUnlocked();
@@ -1144,8 +1163,11 @@ nsBufferedAudioStream::GetPositionInFramesUnlocked()
   }
 
   uint64_t position = 0;
-  if (cubeb_stream_get_position(mCubebStream, &position) != CUBEB_OK) {
-    return -1;
+  {
+    MonitorAutoUnlock mon(mMonitor);
+    if (cubeb_stream_get_position(mCubebStream, &position) != CUBEB_OK) {
+      return -1;
+    }
   }
 
   // Adjust the reported position by the number of silent frames written
@@ -1241,15 +1263,13 @@ nsBufferedAudioStream::DataCallback(void* aBuffer, long aFrames)
 int
 nsBufferedAudioStream::StateCallback(cubeb_state aState)
 {
+  MonitorAutoLock mon(mMonitor);
   if (aState == CUBEB_STATE_DRAINED) {
-    MonitorAutoLock mon(mMonitor);
     mState = DRAINED;
-    mon.NotifyAll();
   } else if (aState == CUBEB_STATE_ERROR) {
-    MonitorAutoLock mon(mMonitor);
     mState = ERRORED;
-    mon.NotifyAll();
   }
+  mon.NotifyAll();
   return CUBEB_OK;
 }
 #endif

content/media/nsAudioStream.h

@@ -11,6 +11,10 @@
 #include "nsIThread.h"
 #include "nsAutoPtr.h"
 
+// Access to a single instance of this class must be synchronized by
+// callers, or made from a single thread.  One exception is that access to
+// GetPosition, GetPositionInFrames, SetVolume, and Get{Rate,Channels,Format}
+// is thread-safe without external synchronization.
 class nsAudioStream : public nsISupports
 {
 public:
@@ -50,11 +54,13 @@ public:
   // Initialize the audio stream. aNumChannels is the number of audio
   // channels (1 for mono, 2 for stereo, etc) and aRate is the sample rate
   // (22050Hz, 44100Hz, etc).
-  // Unsafe to call with the decoder monitor held.
+  // Unsafe to call with a monitor held due to synchronous event execution
+  // on the main thread, which may attempt to acquire any held monitor.
   virtual nsresult Init(PRInt32 aNumChannels, PRInt32 aRate, SampleFormat aFormat) = 0;
 
   // Closes the stream. All future use of the stream is an error.
-  // Unsafe to call with the decoder monitor held.
+  // Unsafe to call with a monitor held due to synchronous event execution
+  // on the main thread, which may attempt to acquire any held monitor.
   virtual void Shutdown() = 0;
 
   // Write audio data to the audio hardware.  aBuf is an array of frames in
@@ -67,11 +73,12 @@ public:
   virtual PRUint32 Available() = 0;
 
   // Set the current volume of the audio playback. This is a value from
-  // 0 (meaning muted) to 1 (meaning full volume).
+  // 0 (meaning muted) to 1 (meaning full volume).  Thread-safe.
   virtual void SetVolume(double aVolume) = 0;
 
   // Block until buffered audio data has been consumed.
-  // Unsafe to call with the decoder monitor held.
+  // Unsafe to call with a monitor held due to synchronous event execution
+  // on the main thread, which may attempt to acquire any held monitor.
   virtual void Drain() = 0;
 
   // Pause audio playback
@@ -81,11 +88,11 @@ public:
   virtual void Resume() = 0;
 
   // Return the position in microseconds of the audio frame being played by
-  // the audio hardware.
+  // the audio hardware.  Thread-safe.
   virtual PRInt64 GetPosition() = 0;
 
   // Return the position, measured in audio frames played since the stream
-  // was opened, of the audio hardware.
+  // was opened, of the audio hardware.  Thread-safe.
   virtual PRInt64 GetPositionInFrames() = 0;
 
   // Returns true when the audio stream is paused.
@@ -93,7 +100,8 @@ public:
 
   // Returns the minimum number of audio frames which must be written before
   // you can be sure that something will be played.
-  // Unsafe to call with the decoder monitor held.
+  // Unsafe to call with a monitor held due to synchronous event execution
+  // on the main thread, which may attempt to acquire any held monitor.
   virtual PRInt32 GetMinWriteSize() = 0;
 
   int GetRate() { return mRate; }