Crash fix: Remove MessageLoop from AudioPowerMonitor and instead use...

Crash fix: Remove MessageLoop from AudioPowerMonitor and instead use MessageLoopProxy in AudioOutputController.

Root cause: AudioPowerMonitor held a reference to the audio thread's MessageLoop and erroneously assumed it would be valid until after the audio stream is closed.  However, at browser shutdown, it's possible for audio streams to be closed by the correct thread, but *after* the MessageLoop associated with the thread is destroyed.

BUG=268629
TEST=media_unittests and manual confirmation by running a browser with the --enable-audible-notifications command-line flag.

Review URL: https://chromiumcodereview.appspot.com/22339024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@217142 0039d316-1c4b-4281-b951-d872f2087c98

media/audio/audio_output_controller.cc

@@ -11,7 +11,6 @@
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
-#include "media/audio/audio_power_monitor.h"
 #include "media/audio/audio_util.h"
 #include "media/audio/shared_memory_util.h"
 #include "media/base/scoped_histogram_timer.h"
@@ -49,7 +48,10 @@ AudioOutputController::AudioOutputController(AudioManager* audio_manager,
       num_allowed_io_(0),
       sync_reader_(sync_reader),
       message_loop_(audio_manager->GetMessageLoop()),
-      number_polling_attempts_left_(0) {
+      number_polling_attempts_left_(0),
+      power_monitor_(
+          params.sample_rate(),
+          TimeDelta::FromMilliseconds(kPowerMeasurementTimeConstantMillis)) {
   DCHECK(audio_manager);
   DCHECK(handler_);
   DCHECK(sync_reader_);
@@ -156,17 +158,13 @@ void AudioOutputController::DoPlay() {
 
   state_ = kPlaying;
 
-  // Start monitoring power levels and send an initial notification that we're
-  // starting in silence.
-  handler_->OnPowerMeasured(AudioPowerMonitor::zero_power(), false);
-  power_monitor_callback_.Reset(
-      base::Bind(&EventHandler::OnPowerMeasured, base::Unretained(handler_)));
-  power_monitor_.reset(new AudioPowerMonitor(
-      params_.sample_rate(),
-      TimeDelta::FromMilliseconds(kPowerMeasurementTimeConstantMillis),
-      TimeDelta::FromSeconds(1) / kPowerMeasurementsPerSecond,
-      base::MessageLoop::current(),
-      power_monitor_callback_.callback()));
+  power_monitor_.Reset();
+  power_poll_callback_.Reset(
+      base::Bind(&AudioOutputController::ReportPowerMeasurementPeriodically,
+                 this));
+  // Run the callback to send an initial notification that we're starting in
+  // silence, and to schedule periodic callbacks.
+  power_poll_callback_.callback().Run();
 
   // We start the AudioOutputStream lazily.
   AllowEntryToOnMoreIOData();
@@ -175,6 +173,16 @@ void AudioOutputController::DoPlay() {
   handler_->OnPlaying();
 }
 
+void AudioOutputController::ReportPowerMeasurementPeriodically() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  const std::pair<float, bool>& reading =
+      power_monitor_.ReadCurrentPowerAndClip();
+  handler_->OnPowerMeasured(reading.first, reading.second);
+  message_loop_->PostDelayedTask(
+      FROM_HERE, power_poll_callback_.callback(),
+      TimeDelta::FromSeconds(1) / kPowerMeasurementsPerSecond);
+}
+
 void AudioOutputController::StopStream() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
@@ -182,11 +190,7 @@ void AudioOutputController::StopStream() {
     stream_->Stop();
     DisallowEntryToOnMoreIOData();
 
-    // Stop monitoring power levels.  By canceling power_monitor_callback_, any
-    // tasks posted to |message_loop_| by AudioPowerMonitor during the
-    // stream_->Stop() call above will not run.
-    power_monitor_.reset();
-    power_monitor_callback_.Cancel();
+    power_poll_callback_.Cancel();
 
     state_ = kPaused;
   }
@@ -279,7 +283,7 @@ int AudioOutputController::OnMoreIOData(AudioBus* source,
   sync_reader_->UpdatePendingBytes(
       buffers_state.total_bytes() + frames * params_.GetBytesPerFrame());
 
-  power_monitor_->Scan(*dest, frames);
+  power_monitor_.Scan(*dest, frames);
 
   AllowEntryToOnMoreIOData();
   return frames;

media/audio/audio_output_controller.h

@@ -9,10 +9,9 @@
 #include "base/callback.h"
 #include "base/cancelable_callback.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/timer/timer.h"
 #include "media/audio/audio_io.h"
 #include "media/audio/audio_manager.h"
+#include "media/audio/audio_power_monitor.h"
 #include "media/audio/audio_source_diverter.h"
 #include "media/audio/simple_sources.h"
 #include "media/base/media_export.h"
@@ -53,8 +52,6 @@
 
 namespace media {
 
-class AudioPowerMonitor;
-
 class MEDIA_EXPORT AudioOutputController
     : public base::RefCountedThreadSafe<AudioOutputController>,
       public AudioOutputStream::AudioSourceCallback,
@@ -182,9 +179,9 @@ class MEDIA_EXPORT AudioOutputController
   void DoStartDiverting(AudioOutputStream* to_stream);
   void DoStopDiverting();
 
-  // Called at regular intervals during playback to check for a change in
-  // silence and call EventHandler::OnAudible() when state changes occur.
-  void MaybeInvokeAudibleCallback();
+  // Calls EventHandler::OnPowerMeasured() with the current power level and then
+  // schedules itself to be called again later.
+  void ReportPowerMeasurementPeriodically();
 
   // Helper method that stops the physical stream.
   void StopStream();
@@ -234,11 +231,11 @@ class MEDIA_EXPORT AudioOutputController
   // Number of times left.
   int number_polling_attempts_left_;
 
-  // Scans audio samples from OnMoreIOData() as input and causes
-  // EventHandler::OnPowerMeasured() to be called with power level measurements
-  // at regular intervals.
-  scoped_ptr<AudioPowerMonitor> power_monitor_;
-  base::CancelableCallback<void(float, bool)> power_monitor_callback_;
+  // Scans audio samples from OnMoreIOData() as input to compute power levels.
+  AudioPowerMonitor power_monitor_;
+
+  // Periodic callback to report power levels during playback.
+  base::CancelableClosure power_poll_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioOutputController);
 };

media/audio/audio_power_monitor.cc

@@ -7,38 +7,28 @@
 #include <algorithm>
 #include <cmath>
 
-#include "base/bind.h"
 #include "base/float_util.h"
 #include "base/logging.h"
-#include "base/message_loop/message_loop.h"
 #include "base/time/time.h"
 #include "media/base/audio_bus.h"
 
 namespace media {
 
 AudioPowerMonitor::AudioPowerMonitor(
-    int sample_rate,
-    const base::TimeDelta& time_constant,
-    const base::TimeDelta& measurement_period,
-    base::MessageLoop* message_loop,
-    const PowerMeasurementCallback& callback)
+    int sample_rate, const base::TimeDelta& time_constant)
     : sample_weight_(
-          1.0f - expf(-1.0f / (sample_rate * time_constant.InSecondsF()))),
-      num_frames_per_callback_(sample_rate * measurement_period.InSecondsF()),
-      message_loop_(message_loop),
-      power_level_callback_(callback),
-      average_power_(0.0f),
-      clipped_since_last_notification_(false),
-      frames_since_last_notification_(0),
-      last_reported_power_(-1.0f),
-      last_reported_clipped_(false) {
-  DCHECK(message_loop_);
-  DCHECK(!power_level_callback_.is_null());
+          1.0f - expf(-1.0f / (sample_rate * time_constant.InSecondsF()))) {
+  Reset();
 }
 
 AudioPowerMonitor::~AudioPowerMonitor() {
 }
 
+void AudioPowerMonitor::Reset() {
+  power_reading_ = average_power_ = 0.0f;
+  clipped_reading_ = has_clipped_ = false;
+}
+
 void AudioPowerMonitor::Scan(const AudioBus& buffer, int num_frames) {
   DCHECK_LE(num_frames, buffer.frames());
   const int num_channels = buffer.channels();
@@ -50,10 +40,10 @@ void AudioPowerMonitor::Scan(const AudioBus& buffer, int num_frames) {
   //
   // TODO(miu): Implement optimized SSE/NEON to more efficiently compute the
   // results (in media/base/vector_math) in soon-upcoming change.
-  bool clipped = false;
   float sum_power = 0.0f;
   for (int i = 0; i < num_channels; ++i) {
     float average_power_this_channel = average_power_;
+    bool clipped = false;
     const float* p = buffer.channel(i);
     const float* const end_of_samples = p + num_frames;
     for (; p < end_of_samples; ++p) {
@@ -64,45 +54,41 @@ void AudioPowerMonitor::Scan(const AudioBus& buffer, int num_frames) {
           (sample_squared - average_power_this_channel) * sample_weight_;
     }
     // If data in audio buffer is garbage, ignore its effect on the result.
-    if (base::IsNaN(average_power_this_channel))
+    if (base::IsNaN(average_power_this_channel)) {
       average_power_this_channel = average_power_;
+      clipped = false;
+    }
     sum_power += average_power_this_channel;
+    has_clipped_ |= clipped;
   }
 
-  // Update accumulated results.
+  // Update accumulated results, with clamping for sanity.
   average_power_ = std::max(0.0f, std::min(1.0f, sum_power / num_channels));
-  clipped_since_last_notification_ |= clipped;
-  frames_since_last_notification_ += num_frames;
-
-  // Once enough frames have been scanned, report the accumulated results.
-  if (frames_since_last_notification_ >= num_frames_per_callback_) {
-    // Note: Forgo making redundant callbacks when results remain unchanged.
-    // Part of this is to pin-down the power to zero if it is insignificantly
-    // small.
-    const float kInsignificantPower = 1.0e-10f;  // -100 dBFS
-    const float power =
-        (average_power_ < kInsignificantPower) ? 0.0f : average_power_;
-    if (power != last_reported_power_ ||
-        clipped_since_last_notification_ != last_reported_clipped_) {
-      const float power_dbfs =
-          power > 0.0f ? 10.0f * log10f(power) : zero_power();
-      // Try to post a task to run the callback with the dBFS result.  The
-      // posting of the task is guaranteed to be non-blocking, and therefore
-      // could fail.  However, in the common case, failures should be rare (and
-      // then the task-post will likely succeed the next time it's attempted).
-      if (!message_loop_->TryPostTask(
-              FROM_HERE,
-              base::Bind(power_level_callback_,
-                         power_dbfs, clipped_since_last_notification_))) {
-        DVLOG(2) << "TryPostTask() did not succeed.";
-        return;
-      }
-      last_reported_power_ = power;
-      last_reported_clipped_ = clipped_since_last_notification_;
+
+  // Push results for reading by other threads, non-blocking.
+  if (reading_lock_.Try()) {
+    power_reading_ = average_power_;
+    if (has_clipped_) {
+      clipped_reading_ = true;
+      has_clipped_ = false;
     }
-    clipped_since_last_notification_ = false;
-    frames_since_last_notification_ = 0;
+    reading_lock_.Release();
   }
 }
 
+std::pair<float, bool> AudioPowerMonitor::ReadCurrentPowerAndClip() {
+  base::AutoLock for_reading(reading_lock_);
+
+  // Convert power level to dBFS units, and pin it down to zero if it is
+  // insignificantly small.
+  const float kInsignificantPower = 1.0e-10f;  // -100 dBFS
+  const float power_dbfs = power_reading_ < kInsignificantPower ? zero_power() :
+      10.0f * log10f(power_reading_);
+
+  const bool clipped = clipped_reading_;
+  clipped_reading_ = false;
+
+  return std::make_pair(power_dbfs, clipped);
+}
+
 }  // namespace media

media/audio/audio_power_monitor.h

@@ -6,8 +6,10 @@
 #define MEDIA_AUDIO_AUDIO_POWER_MONITOR_H_
 
 #include <limits>
+#include <utility>
 
 #include "base/callback.h"
+#include "base/synchronization/lock.h"
 #include "media/base/media_export.h"
 
 // An audio signal power monitor.  It is periodically provided an AudioBus by
@@ -24,7 +26,6 @@
 // undetermined/unbounded amount of run-time.
 
 namespace base {
-class MessageLoop;
 class TimeDelta;
 }
 
@@ -34,31 +35,28 @@ class AudioBus;
 
 class MEDIA_EXPORT AudioPowerMonitor {
  public:
-  // Reports power level in terms of dBFS (see zero_power() and max_power()
-  // below).  |clipped| is true if any *one* sample exceeded maximum amplitude
-  // since the last invocation.
-  typedef base::Callback<void(float power_dbfs, bool clipped)>
-      PowerMeasurementCallback;
-
   // |sample_rate| is the audio signal sample rate (Hz).  |time_constant|
   // characterizes how samples are averaged over time to determine the power
   // level; and is the amount of time it takes a zero power level to increase to
-  // ~63.2% of maximum given a step input signal.  |measurement_period| is the
-  // time length of signal to analyze before invoking the callback to report the
-  // current power level.  |message_loop| is where the |callback| task will be
-  // posted.
-  AudioPowerMonitor(int sample_rate,
-                    const base::TimeDelta& time_constant,
-                    const base::TimeDelta& measurement_period,
-                    base::MessageLoop* message_loop,
-                    const PowerMeasurementCallback& callback);
+  // ~63.2% of maximum given a step input signal.
+  AudioPowerMonitor(int sample_rate, const base::TimeDelta& time_constant);
 
   ~AudioPowerMonitor();
 
+  // Reset power monitor to initial state (zero power level).  This should not
+  // be called while another thread is scanning.
+  void Reset();
+
   // Scan more |frames| of audio data from |buffer|.  It is safe to call this
   // from a real-time priority thread.
   void Scan(const AudioBus& buffer, int frames);
 
+  // Returns the current power level in dBFS and clip status.  Clip status is
+  // true whenever any *one* sample scanned exceeded maximum amplitude since
+  // this method's last invocation.  It is safe to call this method from any
+  // thread.
+  std::pair<float, bool> ReadCurrentPowerAndClip();
+
   // dBFS value corresponding to zero power in the audio signal.
   static float zero_power() { return -std::numeric_limits<float>::infinity(); }
 
@@ -70,23 +68,16 @@ class MEDIA_EXPORT AudioPowerMonitor {
   // |sample_rate| and |time_constant|.
   const float sample_weight_;
 
-  // Number of audio frames to be scanned before reporting the current power
-  // level via callback, as computed from |sample_rate| and
-  // |measurement_period|.
-  const int num_frames_per_callback_;
-
-  // MessageLoop and callback used to notify of the current power level.
-  base::MessageLoop* const message_loop_;
-  const PowerMeasurementCallback power_level_callback_;
-
-  // Accumulated results over one or more calls to Scan().
+  // Accumulated results over one or more calls to Scan().  These should only be
+  // touched by the thread invoking Scan().
   float average_power_;
-  bool clipped_since_last_notification_;
-  int frames_since_last_notification_;
+  bool has_clipped_;
 
-  // Keep track of last reported results to forgo making redundant callbacks.
-  float last_reported_power_;
-  bool last_reported_clipped_;
+  // Copies of power and clip status, used to deliver results synchronously
+  // across threads.
+  base::Lock reading_lock_;
+  float power_reading_;
+  bool clipped_reading_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioPowerMonitor);
 };

media/audio/audio_power_monitor_unittest.cc

@@ -6,9 +6,6 @@
 
 #include <limits>
 
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/message_loop/message_loop.h"
 #include "base/time/time.h"
 #include "media/base/audio_bus.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -19,7 +16,6 @@ static const int kSampleRate = 48000;
 static const int kFramesPerBuffer = 128;
 
 static const int kTimeConstantMillis = 5;
-static const int kMeasurementPeriodMillis = 20;
 
 namespace {
 
@@ -158,26 +154,22 @@ class MeasurementObserver {
 class AudioPowerMonitorTest : public ::testing::TestWithParam<TestScenario> {
  public:
   AudioPowerMonitorTest()
-      : power_monitor_(
-            kSampleRate,
-            base::TimeDelta::FromMilliseconds(kTimeConstantMillis),
-            base::TimeDelta::FromMilliseconds(kMeasurementPeriodMillis),
-            &message_loop_,
-            base::Bind(&AudioPowerMonitorTest::OnPowerMeasured,
-                       base::Unretained(this))) {}
+      : power_monitor_(kSampleRate,
+                       base::TimeDelta::FromMilliseconds(kTimeConstantMillis)) {
+  }
 
   void FeedAndCheckExpectedPowerIsMeasured(
       const AudioBus& bus, float power, bool clipped) {
-    // Feed the AudioPowerMonitor.  It should post tasks to |message_loop_|.
+    // Feed the AudioPowerMonitor, read measurements from it, and record them in
+    // MeasurementObserver.
     static const int kNumFeedIters = 100;
-    for (int i = 0; i < kNumFeedIters; ++i)
-      power_monitor_.Scan(bus, bus.frames());
-
-    // Set up an observer and run all the enqueued tasks.
     MeasurementObserver observer(power, clipped);
-    current_observer_ = &observer;
-    message_loop_.RunUntilIdle();
-    current_observer_ = NULL;
+    for (int i = 0; i < kNumFeedIters; ++i) {
+      power_monitor_.Scan(bus, bus.frames());
+      const std::pair<float, bool>& reading =
+          power_monitor_.ReadCurrentPowerAndClip();
+      observer.OnPowerMeasured(reading.first, reading.second);
+    }
 
     // Check that the results recorded by the observer are the same whole-number
     // dBFS.
@@ -187,14 +179,7 @@ class AudioPowerMonitorTest : public ::testing::TestWithParam<TestScenario> {
   }
 
  private:
-  void OnPowerMeasured(float power, bool clipped) {
-    CHECK(current_observer_);
-    current_observer_->OnPowerMeasured(power, clipped);
-  }
-
-  base::MessageLoop message_loop_;
   AudioPowerMonitor power_monitor_;
-  MeasurementObserver* current_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioPowerMonitorTest);
 };
@@ -303,7 +288,7 @@ INSTANTIATE_TEST_CASE_P(
          TestScenario(kMonoMaxAmplitudeWithClip2, 1, 4,
                       AudioPowerMonitor::max_power(), true),
          TestScenario(kMonoSilentNoise, 1, 2,
-                      AudioPowerMonitor::zero_power(), true).
+                      AudioPowerMonitor::zero_power(), false).
              WithABadSample(std::numeric_limits<float>::infinity()),
          TestScenario(kMonoHalfMaxAmplitude, 1, 4,
                       AudioPowerMonitor::zero_power(), false).