Bug 1460346 - Verify that AudioMixer'callback is removed before the destruction of GraphDriver. r=padenot
authorAlex Chronopoulos <achronop@gmail.com>
Fri, 29 Jun 2018 10:05:56 +0200
changeset 479729 656a0868f578d3012165dacf2e4f89ac6be7002c
parent 479728 6105079ea2acad19899d21dc4a1b586f4a0d6dd0
child 479730 2862673105782aa71878aa2074cf61c8cfba915e
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspadenot
bugs1460346
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1460346 - Verify that AudioMixer'callback is removed before the destruction of GraphDriver. r=padenot MozReview-Commit-ID: Jd0PVw7xHDV
dom/media/GraphDriver.cpp
dom/media/GraphDriver.h
dom/media/MediaStreamGraph.cpp
--- a/dom/media/GraphDriver.cpp
+++ b/dom/media/GraphDriver.cpp
@@ -562,16 +562,17 @@ AudioCallbackDriver::AudioCallbackDriver
     audio::AudioNotificationReceiver::Register(this);
   }
 #endif
 }
 
 AudioCallbackDriver::~AudioCallbackDriver()
 {
   MOZ_ASSERT(mPromisesForOperation.IsEmpty());
+  MOZ_ASSERT(!mAddedMixer);
 #if defined(XP_WIN)
   if (XRE_IsContentProcess()) {
     audio::AudioNotificationReceiver::Unregister(this);
   }
 #endif
 }
 
 bool IsMacbookOrMacbookAir()
@@ -793,38 +794,50 @@ AudioCallbackDriver::Revive()
 {
   MOZ_ASSERT(NS_IsMainThread() && !ThreadRunning());
   // Note: only called on MainThread, without monitor
   // We know were weren't in a running state
   LOG(LogLevel::Debug, ("AudioCallbackDriver reviving."));
   // If we were switching, switch now. Otherwise, start the audio thread again.
   MonitorAutoLock mon(mGraphImpl->GetMonitor());
   if (NextDriver()) {
-    RemoveCallback();
     SwitchToNextDriver();
   } else {
     LOG(LogLevel::Debug,
         ("Starting audio threads for MediaStreamGraph %p from a new thread.",
          mGraphImpl));
     RefPtr<AsyncCubebTask> initEvent =
       new AsyncCubebTask(this, AsyncCubebOperation::INIT);
     initEvent->Dispatch();
   }
 }
 
 void
-AudioCallbackDriver::RemoveCallback()
+AudioCallbackDriver::RemoveMixerCallback()
 {
+  MOZ_ASSERT(OnThread() || !ThreadRunning());
+
   if (mAddedMixer) {
     mGraphImpl->mMixer.RemoveCallback(this);
     mAddedMixer = false;
   }
 }
 
 void
+AudioCallbackDriver::AddMixerCallback()
+{
+  MOZ_ASSERT(OnThread());
+
+  if (!mAddedMixer) {
+    mGraphImpl->mMixer.AddCallback(this);
+    mAddedMixer = true;
+  }
+}
+
+void
 AudioCallbackDriver::WaitForNextIteration()
 {
 }
 
 void
 AudioCallbackDriver::WakeUp()
 {
   mGraphImpl->GetMonitor().AssertCurrentThreadOwns();
@@ -896,20 +909,17 @@ AudioCallbackDriver::DataCallback(const 
    TRACE_AUDIO_CALLBACK_BUDGET(aFrames, mSampleRate);
    TRACE_AUDIO_CALLBACK();
 
 #ifdef DEBUG
   AutoInCallback aic(this);
 #endif
 
   // Don't add the callback until we're inited and ready
-  if (!mAddedMixer) {
-    mGraphImpl->mMixer.AddCallback(this);
-    mAddedMixer = true;
-  }
+  AddMixerCallback();
 
   GraphTime stateComputedTime = StateComputedTime();
   if (stateComputedTime == 0) {
     MonitorAutoLock mon(mGraphImpl->GetMonitor());
     // Because this function is called during cubeb_stream_init (to prefill the
     // audio buffers), it can be that we don't have a message here (because this
     // driver is the first one for this graph), and the graph would exit. Simply
     // return here until we have messages.
@@ -1009,16 +1019,17 @@ AudioCallbackDriver::DataCallback(const 
   if (!stillProcessing) {
     // About to hand over control of the graph.  Do not start a new driver if
     // StateCallback() receives an error for this stream while the main thread
     // or another driver has control of the graph.
     mShouldFallbackIfError = false;
     // Enter shutdown mode. The stable-state handler will detect this
     // and complete shutdown if the graph does not get restarted.
     mGraphImpl->SignalMainThreadCleanup();
+    RemoveMixerCallback();
     // Update the flag before go to drain
     mAudioThreadRunning = false;
     return aFrames - 1;
   }
 
   bool switching = false;
   {
     MonitorAutoLock mon(mGraphImpl->GetMonitor());
@@ -1029,17 +1040,17 @@ AudioCallbackDriver::DataCallback(const 
     mShouldFallbackIfError = false;
     // If the audio stream has not been started by the previous driver or
     // the graph itself, keep it alive.
     MonitorAutoLock mon(mGraphImpl->GetMonitor());
     if (!IsStarted()) {
       return aFrames;
     }
     LOG(LogLevel::Debug, ("Switching to system driver."));
-    RemoveCallback();
+    RemoveMixerCallback();
     SwitchToNextDriver();
     mAudioThreadRunning = false;
     // Returning less than aFrames starts the draining and eventually stops the
     // audio thread. This function will never get called again.
     return aFrames - 1;
   }
 
   return aFrames;
@@ -1051,20 +1062,24 @@ AudioCallbackDriver::StateCallback(cubeb
   MOZ_ASSERT(!OnThread());
   LOG(LogLevel::Debug, ("AudioCallbackDriver State: %d", aState));
 
   // Clear the flag for the not running
   // states: stopped, drained, error.
   mAudioThreadRunning = (aState == CUBEB_STATE_STARTED);
 
   if (aState == CUBEB_STATE_ERROR && mShouldFallbackIfError) {
+    MOZ_ASSERT(!ThreadRunning());
     mShouldFallbackIfError = false;
     MonitorAutoLock lock(GraphImpl()->GetMonitor());
-    RemoveCallback();
+    RemoveMixerCallback();
     FallbackToSystemClockDriver();
+  } else if (aState == CUBEB_STATE_STOPPED) {
+    MOZ_ASSERT(!ThreadRunning());
+    RemoveMixerCallback();
   }
 }
 
 void
 AudioCallbackDriver::MixerCallback(AudioDataValue* aMixedBuffer,
                                    AudioSampleFormat aFormat,
                                    uint32_t aChannels,
                                    uint32_t aFrames,
--- a/dom/media/GraphDriver.h
+++ b/dom/media/GraphDriver.h
@@ -488,17 +488,19 @@ public:
   /* Tell the driver whether this process is using a microphone or not. This is
    * thread safe. */
   void SetMicrophoneActive(bool aActive);
 
   void CompleteAudioContextOperations(AsyncCubebOperation aOperation);
 
 private:
   /* Remove Mixer callbacks when switching */
-  void RemoveCallback() ;
+  void RemoveMixerCallback();
+  /* Add this driver in Mixer callbacks. */
+  void AddMixerCallback();
   /**
    * On certain MacBookPro, the microphone is located near the left speaker.
    * We need to pan the sound output to the right speaker if we are using the
    * mic and the built-in speaker, or we will have terrible echo.  */
   void PanOutputIfNeeded(bool aMicrophoneActive);
   /**
    * This is called when the output device used by the cubeb stream changes. */
   void DeviceChangedCallback();
@@ -568,18 +570,21 @@ private:
     AudioCallbackDriver* mDriver;
   };
 
   /* Shared thread pool with up to one thread for off-main-thread
    * initialization and shutdown of the audio stream via AsyncCubebTask. */
   const RefPtr<SharedThreadPool> mInitShutdownThread;
   /* This must be accessed with the graph monitor held. */
   AutoTArray<StreamAndPromiseForOperation, 1> mPromisesForOperation;
-  /* Used to queue us to add the mixer callback on first run. */
-  bool mAddedMixer;
+  /* This is used to signal adding the mixer callback on first run
+   * of audio callback. This is atomic because it is touched from different
+   * threads, the audio callback thread and the state change thread. However,
+   * the order of the threads does not allow concurent access. */
+  Atomic<bool> mAddedMixer;
 
   /* Contains the id of the audio thread for as long as the callback
    * is taking place, after that it is reseted to an invalid value. */
   std::atomic<std::thread::id> mAudioThreadId;
   /* True when audio thread is running. False before
    * starting and after stopping it the audio thread. */
   Atomic<bool> mAudioThreadRunning;
   /**
--- a/dom/media/MediaStreamGraph.cpp
+++ b/dom/media/MediaStreamGraph.cpp
@@ -678,17 +678,17 @@ MediaStreamGraphImpl::CreateOrDestroyAud
       aStream->mAudioOutputStreams.RemoveElementAt(i);
     }
   }
 }
 
 StreamTime
 MediaStreamGraphImpl::PlayAudio(MediaStream* aStream)
 {
-  MOZ_ASSERT(OnGraphThreadOrNotRunning());
+  MOZ_ASSERT(OnGraphThread());
   MOZ_ASSERT(mRealtime, "Should only attempt to play audio in realtime mode");
 
   float volume = 0.0f;
   for (uint32_t i = 0; i < aStream->mAudioOutputs.Length(); ++i) {
     volume += aStream->mAudioOutputs[i].mVolume;
   }
 
   StreamTime ticksWritten = 0;