Crash fix: Remove MessageLoop from AudioPowerMonitor and instead use MessageLoopProxy in AudioOutpu…

media/audio/audio_power_monitor.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/audio/audio_power_monitor.h"
 
 #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/third_party/dynamic_annotations/dynamic_annotations.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()))) {
+  ANNOTATE_BENIGN_RACE(

[DaleCurtis, 2013/08/12 18:23:29]

Annotating only keeps TSAN happy, while the impl is still racy / relies on
undefined behavior, which is potentially problematic across the weakly ordered
architectures supported by Chrome.

While not the end of the world in this case, a Lock::Try() in Scan() + Lock in
Reset() + Read() would make this race free w/o a loss of clarity or problematic
overhead.

[miu, 2013/08/12 19:41:46]

Done.

+      &average_power_, "only one writer; reader takes samples periodically");
+  ANNOTATE_BENIGN_RACE(
+      &has_clipped_,
+      "flag set by one thread, cleared by the other; okay to be out-of-sync");
+  Reset();
 }
 
 AudioPowerMonitor::~AudioPowerMonitor() {
 }
 
+void AudioPowerMonitor::Reset() {
+  average_power_ = 0.0f;
+  has_clipped_ = false;
+}
+
 void AudioPowerMonitor::Scan(const AudioBus& buffer, int num_frames) {
   DCHECK_LE(num_frames, buffer.frames());
   const int num_channels = buffer.channels();
   if (num_frames <= 0 || num_channels <= 0)
     return;
 
   // Calculate a new average power by applying a first-order low-pass filter
   // over the audio samples in |buffer|.
   //
   // TODO(miu): Implement optimized SSE/NEON to more efficiently compute the
@@ skipping 10 matching lines @@
       clipped |= (sample_squared > 1.0f);
       average_power_this_channel +=
           (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))
       average_power_this_channel = average_power_;
     sum_power += average_power_this_channel;
   }
 
   // Update accumulated results.

[DaleCurtis, 2013/08/12 18:23:29]

if (lock.Try()) {
  average_power_exported_ = average_power_;
  clipped_exported_ |= clipped_;
  lock.Release();
}

Alternatively, check out the AutoTryLock class in WebAudioSourceProviderImpl().

[miu, 2013/08/12 19:41:46]

Done.

   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;
+  if (clipped)

[DaleCurtis, 2013/08/12 18:23:29]

This is not the same behavior as before and will lead to loss of clipped status,
no?

[miu, 2013/08/12 19:41:46]

It looks like the same behavior to me.  Can you provide a counter-example?

The change is mostly to make it obvious in the code that the clip flag is set by
one thread, and cleared (i.e., "consumed") by another.

[DaleCurtis, 2013/08/12 20:43:56]

Ah you're right.  The | is clearer to me apparently :p

+    has_clipped_ = true;
+}
 
-  // 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_;
-    }
-    clipped_since_last_notification_ = false;
-    frames_since_last_notification_ = 0;
-  }
+float AudioPowerMonitor::ReadCurrentPower() const {
+  // Ensure we have a copy of |average_power_| local to this thread so that the
+  // value will not mutate while evaluating the return expression below.
+  float sampled_power;
+  memcpy(&sampled_power, &average_power_, sizeof(float));

[DaleCurtis, 2013/08/12 18:23:29]

I don't think memcpy provides you any guarantees, so I assume you're trying to
avoid the compiler optimizing away the local value?  Sketchy :)

[miu, 2013/08/12 19:41:46]

Removed.  Not needed anymore now that we're using locking.

+
+  // Return the sampled power level, converted to dBFS units, and pinned-down to
+  // zero if it is insignificantly small.
+  const float kInsignificantPower = 1.0e-10f;  // -100 dBFS
+  return sampled_power < kInsignificantPower ? zero_power() :
+      10.0f * log10f(sampled_power);
+}
+
+bool AudioPowerMonitor::TestForClippingAndClear() {
+  const bool clipped = has_clipped_;
+  if (clipped)
+    has_clipped_ = false;
+  return clipped;
 }
 
 }  // namespace media