Replace AudioSilenceDetector with an AudioPowerMonitor.

Replace AudioSilenceDetector with an AudioPowerMonitor.

This will allow the tab audio UI indicator to animate/paint based on the real-time measured power level of the audio stream being played.  This change includes necessary plumbing of the power measurement value all the way from audio internals to the fringes of the tab UI.

BUG=178934
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213042

[miu, 2013-05-16 05:35:35]

Chris/Dale,

These are the audio internals changes needed to support a real-time power level
display in the tab audio UI indicator (see bug for more details).

I took the old solution (silence detection), and replaced it with a simple power
measurement utility class, AudioPowerMonitor.

crogers: Please review media/audio/audio_power_monitor*.  It is heavily based on
the audio_silence_detector* files I'm deleting in this change.  The Scan()
method is meant to be executed on the native audio thread.  Also, please check
my conversions to dBFS units (I assumed power == amplitude^2).

dalecurtis: If you'd be so kind, please take a look at the rest.  You're in all
the OWNERS files.  ;-)  Feel free to also take a look at the
audio_power_monitor.* files.  More eyes the better.  I've already got some code
cooking to introduce SSE/NEON optimized calculations to improve performance, as
we discussed over lunch.

Thanks,
Yuri

[DaleCurtis, 2013-05-16 18:24:45]

Generally I also wonder if instead of DBFS we should just use a [0,1] range like
volume.  It's hard to see exactly how this dbfs value is being used?  Perhaps
I'm just blind, but I don't see you actually making any comparisons on the power
level?

https://codereview.chromium.org/14600025/diff/2001/chrome/browser/media/audio...
File chrome/browser/media/audio_stream_indicator.cc (right):

https://codereview.chromium.org/14600025/diff/2001/chrome/browser/media/audio...
chrome/browser/media/audio_stream_indicator.cc:43: float
AudioStreamIndicator::GetAudioPowerLevelInDBFS(
Seems like there should be a simpler way to do this, but I can't workout an
(accurate) running average or maximum algorithm without the added overhead for
when a stream stops playing.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_output_c...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_output_c...
media/audio/audio_output_controller.cc:25: static const float
kPowerMeasurementPeriodSeconds = 50.0f / 1000.0f;
Confusing, this doesn't look like it's in seconds? And you use FromMilliseconds
below?

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor.cc:19: static const float kReferenceLevel =
1.0f;
AudioBus doesn't guarantee 1.0 is maximum.  See audio_bus.h:

"Warning: Do not rely on having sane (i.e. not inf, nan, or between [-1.0, 1.0])
values in the channel data."

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor.cc:43: if (!buffer)
Is this ever true?

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor.cc:50: // The overall goal here is to compute
the RMS of the amplitudes over all
You say RMS, but I see no sqrt() below :)

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor.cc:78: if (frames_so_far_ >=
num_frames_per_callback_) {
The way this is written you end up with a stretched sample space if TryPostTask
fails.  Can you elaborate on why you shouldn't just throw away the results for
sample ranges which were unpostable?

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor.cc:81: 10.0f * log10(average_power /
(kReferenceLevel * kReferenceLevel));
10.0? http://en.wikipedia.org/wiki/DBFS says 20.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
File media/audio/audio_power_monitor_unittest.cc (right):

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor_unittest.cc:83: scoped_ptr<AudioBus> bus =
AudioBus::Create(num_channels, kFramesPerBuffer);
Instead of memcpy, etc, you could just create a wrapped audio bus and
SetChannelData()? You would just need to declare the arrays aligned 16 below.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor_unittest.cc:240: 
A test case with values outside of [-1, 1] would be good.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor_unittest.cc:265: }  // namespace
This is a bit weird?  For media/ we generally leave everything in the media
namespace and static const values.

[miu, 2013-05-16 21:44:10]

Thanks, Dale.  I've addressed your comments.

On the topic of using dBFS: Chris suggested in the original AudioSilenceDetector
implementation that we should be using dBFS units when comparing to the "silence
threshold."  In this change, IMHO, it made even more sense to use them.  From
the UI's perspective, dBFS can linearly translate to number of pixels tall/wide
the indicator is painted and this will visually match the user's perception of
the audio power level (i.e., how loud it sounds in their logarithmic-sensing
ears).

I can also see your point about linearly translating the dBFS value to a [0,1]
scale before posting the value to the UI consumer.

Perhaps Chris can chime in here with an opinion on all this?

https://codereview.chromium.org/14600025/diff/2001/chrome/browser/media/audio...
File chrome/browser/media/audio_stream_indicator.cc (right):

https://codereview.chromium.org/14600025/diff/2001/chrome/browser/media/audio...
chrome/browser/media/audio_stream_indicator.cc:43: float
AudioStreamIndicator::GetAudioPowerLevelInDBFS(
On 2013/05/16 18:24:45, DaleCurtis wrote:
> Seems like there should be a simpler way to do this, but I can't workout an
> (accurate) running average or maximum algorithm without the added overhead for
> when a stream stops playing.

Acknowledged.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_output_c...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_output_c...
media/audio/audio_output_controller.cc:25: static const float
kPowerMeasurementPeriodSeconds = 50.0f / 1000.0f;
On 2013/05/16 18:24:45, DaleCurtis wrote:
> Confusing, this doesn't look like it's in seconds? And you use
FromMilliseconds
> below?

Doh!  I had already fixed that before requesting the code review.  Just forgot
to upload the latest patch set.  Trololololo...

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor.cc:19: static const float kReferenceLevel =
1.0f;
On 2013/05/16 18:24:45, DaleCurtis wrote:
> AudioBus doesn't guarantee 1.0 is maximum.  See audio_bus.h:
> 
> "Warning: Do not rely on having sane (i.e. not inf, nan, or between [-1.0,
1.0])
> values in the channel data."

This is the way dBFS units are defined, not a restriction on what we expect to
see in AudioBus data.  But, good point on sanity-checking: I have the
DCHECK(is_finite(x)) already, but the program shouldn't crash on bad data at
run-time.  I've also added clipping as another protection.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor.cc:43: if (!buffer)
On 2013/05/16 18:24:45, DaleCurtis wrote:
> Is this ever true?

Good point.  (Left over from AudioSilenceDetector code.)  I also changed the
signature of the Scan() method to take a const AudioBus& instead.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor.cc:50: // The overall goal here is to compute
the RMS of the amplitudes over all
On 2013/05/16 18:24:45, DaleCurtis wrote:
> You say RMS, but I see no sqrt() below :)

The subtlety: RMS is root-mean-square amplitude.  dBFS uses amplitude^2 (i.e.,
power = amplitude^2).  So, the sqrt() operation was unnecessary when converting
to dBFS units.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor.cc:78: if (frames_so_far_ >=
num_frames_per_callback_) {
On 2013/05/16 18:24:45, DaleCurtis wrote:
> The way this is written you end up with a stretched sample space if
TryPostTask
> fails.  Can you elaborate on why you shouldn't just throw away the results for
> sample ranges which were unpostable?

I put the answer to this in the form of comments in the code, since it's a good
thing to have explained for future code readers:

  // ...  The posting of the task is guaranteed to be
  // non-blocking, and therefore could fail.  If that happens, the accumulated
  // values will be retained.  We expect this to be a good policy since
  // TryPostTask() should rarely fail.  Therefore, only rarely will the
  // measurement period be stretched, and only rarely will it be stretched by
  // more than one extra AudioBus' worth of frames.

Also note: If we throw away results, we have to start over.  That literally
means an additional "measurement period" must elapse before we attempt to post
another task to run the callback.  Users might see this in the form of
"jitteriness" in the smoothness of the tab UI animation.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor.cc:81: 10.0f * log10(average_power /
(kReferenceLevel * kReferenceLevel));
On 2013/05/16 18:24:45, DaleCurtis wrote:
> 10.0? http://en.wikipedia.org/wiki/DBFS says 20.

20 is for amplitudes.  10 is for power (i.e., amplitude^2).  This is mentioned
in http://en.wikipedia.org/wiki/Decibel#Power_quantities

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
File media/audio/audio_power_monitor_unittest.cc (right):

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor_unittest.cc:83: scoped_ptr<AudioBus> bus =
AudioBus::Create(num_channels, kFramesPerBuffer);
On 2013/05/16 18:24:45, DaleCurtis wrote:
> Instead of memcpy, etc, you could just create a wrapped audio bus and
> SetChannelData()? You would just need to declare the arrays aligned 16 below.

This method produces an AudioBus of kFramesPerBuffer in length from the test
data which is num_frames in length.  num_frames is usually only 1-10 samples
(see constants starting on line 183).  Therefore, I need the memcpy() to repeat
the test data to fill the whole buffer.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor_unittest.cc:240: 
On 2013/05/16 18:24:45, DaleCurtis wrote:
> A test case with values outside of [-1, 1] would be good.

Done.  And also added ones containing infinity and NaN.

https://codereview.chromium.org/14600025/diff/2001/media/audio/audio_power_mo...
media/audio/audio_power_monitor_unittest.cc:265: }  // namespace
On 2013/05/16 18:24:45, DaleCurtis wrote:
> This is a bit weird?  For media/ we generally leave everything in the media
> namespace and static const values.

Yes and no.  I actually came up against the following problem when working on
media unit tests in the past: Two different unit test modules declared a class
called MockAudioOutputStream with different implementations.  The linker for
media_unittests picked one of them to use for both tests and then, as expected,
one of the test suites crashed at run-time.

The solution is that test code should be declared in an anonymous namespace as a
precaution.  Though, to match with media coding style, I've put back all the
constants to static and only use the anonymous namespace around the helper class
definitions.

[miu, 2013-05-16 21:45:45]

mark: Could you please review my changes in base/float_util.h and base/time.h? 
I added a IsNaN() function alongside the IsFinite() function.

Thanks,
Yuri

[Mark Mentovai, 2013-05-16 22:38:51]

LGTM in base.

Since we’re evidently including C++’s standard <cmath> on all platforms now, do
std::isnan and std::isfinite work on Windows? If so, we can get rid of
float_util.h and just use the <cmath> routines directly in all code.

[Chris Rogers, 2013-05-16 22:39:02]

some initial comments

https://codereview.chromium.org/14600025/diff/13001/media/audio/audio_power_m...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/14600025/diff/13001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:65: }
Instead of computing RMS power here in this inner loop, it's a lot better to
average over a long time period.

You can do this as follows by using a member state variable ave_power_
(initialized to zero), then:

for (; p < end_of_samples; ++p) {
  float sample = *p;  // avoid computing *p twice which is inefficient
  ave_power_ += (ave_power_ - sample*sample) * k;
}

// k is a time-constant value which is relatively small, for example 0.001

See the discreteTimeConstantForSampleRate() method in:
https://svn.webkit.org/repository/webkit/trunk/Source/WebCore/platform/audio/...

to see how to calculate this value for any "time-constant" at any sample-rate

This implements a 1st order low-pass filter which will smooth the sample values
and will retain the smoothing state across calls to Scan().

https://codereview.chromium.org/14600025/diff/13001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:72: }
For clipping, you need to check every single sample to make sure it does not
exceed the full-scale (range from -1 -> +1).  If even one sample exceeds that
range then you'll want to implement a peak-and-hold (register the clip and hold
the UI red for a second, then release)

Checking each sample is easy, since you just need to loop through all of them
and compare fabs(sample) > 1.0

[Chris Rogers, 2013-05-16 22:40:15]

sorry that should have been:
ave_power_ += (sample*sample - ave_power_) * k;

[miu, 2013-05-16 23:15:36]

On 2013/05/16 22:38:51, Mark Mentovai wrote:
> LGTM in base.
> 
> Since we’re evidently including C++’s standard <cmath> on all platforms now,
do
> std::isnan and std::isfinite work on Windows? If so, we can get rid of
> float_util.h and just use the <cmath> routines directly in all code.

mark: Unfortunately std::isnan and isfinite are officially C++11 additions. 
They tend to be present on any POSIX-based platform, but for Windows MSVC++ 2010
does not define them in <cmath>.

[miu, 2013-05-16 23:23:37]

On 2013/05/16 22:40:15, Chris Rogers wrote:
> sorry that should have been:
> ave_power_ += (sample*sample - ave_power_) * k;

Thanks, Chris.  I like your method for the smoothed average power calculation. 
I'll work on that and add clipping detection as well.

Any comment on the use of dBFS units?  And, whether I should translate to a
[0,1] scale for upstream UI code?

[Chris Rogers, 2013-05-17 00:05:48]

On 2013/05/16 23:23:37, Yuri wrote:
> On 2013/05/16 22:40:15, Chris Rogers wrote:
> > sorry that should have been:
> > ave_power_ += (sample*sample - ave_power_) * k;
> 
> Thanks, Chris.  I like your method for the smoothed average power calculation.

> I'll work on that and add clipping detection as well.
> 
> Any comment on the use of dBFS units?  And, whether I should translate to a
> [0,1] scale for upstream UI code?

For UI a [0,1] value seems ok.  Check out this audio power meter:
http://www.jiscdigitalmedia.ac.uk/images/srt04vumeter.jpg

and notice that even though it's marked in dB units, they're not linearly
spaced.  So there's a certain art to what "curve" you put these power values
into so that the display of the information looks good, and different companies
do it differently.  I suspect you'll have to experiment with some different
curves (creating a final value [0,1]) so that it translates well to UI.  You'll
need to calibrate it so the softest sound you want to register on the UI is not
too soft and not too loud, etc.

[miu, 2013-05-17 01:12:28]

On 2013/05/17 00:05:48, Chris Rogers wrote:
> > Any comment on the use of dBFS units?  And, whether I should translate to a
> 
> do it differently.  I suspect you'll have to experiment with some different
> curves (creating a final value [0,1]) so that it translates well to UI. 
You'll
> need to calibrate it so the softest sound you want to register on the UI is
not
> too soft and not too loud, etc.

Okay.  This makes me want to plumb dBFS to the fringe of the UI code (i.e.,
exactly what I'm doing in this CL), and then leave the calibration curve up to
the UI impl.  UI design/development, not the audio internals, is where we should
be considering how look-and-feel jive with the user's audio perception.

The analogy with your analog meter graphic: The internals of the device are
sending a voltage signal to the meter, and the meter (the UI) provides the
appropriate user-friendly visual interpretation.

[miu, 2013-07-02 06:10:16]

Dale/Chris: Finally got the chance to continue work on this.  Replaced the RMS
power calculations with the 1st-order low-pass filter as Chris suggested.  Made
the units tests a lot simpler/cleaner.  PTAL.

Thanks,
Yuri

https://codereview.chromium.org/14600025/diff/13001/media/audio/audio_power_m...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/14600025/diff/13001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:65: }
On 2013/05/16 22:39:02, Chris Rogers wrote:
> Instead of computing RMS power here in this inner loop, it's a lot better to
> average over a long time period.
> 
> You can do this as follows by using a member state variable ave_power_
> (initialized to zero), then:
> 
> for (; p < end_of_samples; ++p) {
>   float sample = *p;  // avoid computing *p twice which is inefficient
>   ave_power_ += (ave_power_ - sample*sample) * k;
> }
> 
> // k is a time-constant value which is relatively small, for example 0.001
> 
> See the discreteTimeConstantForSampleRate() method in:
>
https://svn.webkit.org/repository/webkit/trunk/Source/WebCore/platform/audio/...
> 
> to see how to calculate this value for any "time-constant" at any sample-rate
> 
> This implements a 1st order low-pass filter which will smooth the sample
values
> and will retain the smoothing state across calls to Scan().

Done.

https://codereview.chromium.org/14600025/diff/13001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:72: }
On 2013/05/16 22:39:02, Chris Rogers wrote:
> For clipping, you need to check every single sample to make sure it does not
> exceed the full-scale (range from -1 -> +1).  If even one sample exceeds that
> range then you'll want to implement a peak-and-hold (register the clip and
hold
> the UI red for a second, then release)
> 
> Checking each sample is easy, since you just need to loop through all of them
> and compare fabs(sample) > 1.0

Good catch.  Done.

[DaleCurtis, 2013-07-02 22:26:34]

https://codereview.chromium.org/14600025/diff/53001/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.h (right):

https://codereview.chromium.org/14600025/diff/53001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:26: float power_dBFS,
Style should be all lowercase I think.

https://codereview.chromium.org/14600025/diff/53001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:45: struct RenderViewId {
If you just use std::pair<int, int> you can avoid a lot of code and take
advantage of binary savings via folding with other maps using pairs of ints.

https://codereview.chromium.org/14600025/diff/53001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:74: typedef std::map<int,
SignalPower> StreamPowerLevelMap;
About how many streams/renderview are we expecting? I wonder if just using a
vector of structs here would be more efficient than a nested map.

Alternatively, I think if you're going make clipped sticky across calls, you can
extend the concept to power levels.  What do you think about this:

struct TabPowerLevel { bool clipped; set<float> levels; }
typedef map<std::pair<int, int>, TabPowerLevel> RenderViewStreamMap;

On Update(), or the clip flag and add the power level to the set. Each call to
CurrentAudibleLevel() call clears the set and clipped flag.  To return the max
power level you just take set.rbegin() since it's ordered.  If empty, assume
silence.

This should be fine so long as CurrentAudibleLevel is called regularly.  As a
sanity check, if the set gets too large you can clear it during update.

https://codereview.chromium.org/14600025/diff/53001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/14600025/diff/53001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:249: #ifndef NDEBUG
As nice as it looks, you should probably remove prior to commit.

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:22: static const float kReferenceLevel =
1.0f;
enum instead?

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:24: const float
AudioPowerMonitor::kZeroPowerDBFS =
Hmm, I'm think this is a static initializer.

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:27: const float
AudioPowerMonitor::kMaxPowerDBFS = 0.0f;
enum?

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:91: const float power = (average_power_ <
1.0e-10f) ? 0.0f : average_power_;
Should probably be a top level const.

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
File media/audio/audio_power_monitor_unittest.cc (right):

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor_unittest.cc:239: 0.0f, 0.0f, 0.0f,
std::numeric_limits<float>::infinity()
Static initializer.

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor_unittest.cc:243: 0.5f, -0.5f, 0.5f,
std::numeric_limits<float>::quiet_NaN()
Ditto.

[DaleCurtis, 2013-07-02 22:26:54]

Also, I defer to crogers for the math in audio power monitor.

[miu, 2013-07-09 00:59:55]

Thanks for the review, Dale.

https://codereview.chromium.org/14600025/diff/53001/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.h (right):

https://codereview.chromium.org/14600025/diff/53001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:26: float power_dBFS,
On 2013/07/02 22:26:34, DaleCurtis wrote:
> Style should be all lowercase I think.

Done.

https://codereview.chromium.org/14600025/diff/53001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:45: struct RenderViewId {
On 2013/07/02 22:26:34, DaleCurtis wrote:
> If you just use std::pair<int, int> you can avoid a lot of code and take
> advantage of binary savings via folding with other maps using pairs of ints.

Done.

I absolutely agree.  There was a past discussion on a previous code review; but
IIRC, it centered strictly around one reviewer's style preference.  I think use
of std::pair<> here has technical merit and is inline with the "eliminate code
bloat" philosophy of the Chromium project.

https://codereview.chromium.org/14600025/diff/53001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:74: typedef std::map<int,
SignalPower> StreamPowerLevelMap;
On 2013/07/02 22:26:34, DaleCurtis wrote:
> I wonder if just using a
> vector of structs here would be more efficient than a nested map.
> ...
> Alternatively, I think if you're going make clipped sticky across calls, you
can
> extend the concept to power levels.
> ...

Took your advice on eliminating the nested map since the find() operations
should be efficient w/ low numbers of streams (as expected).  No more useless
sorting.  ;-)

I realized I was doing the wrong thing here w.r.t. sticky-then-clear handling of
the clip flag.  The reason is that Update() will only be called when the power
level/clip state changes.  So, for example, if sound is continuously clipping at
max power, there will never be any calls to Update() until that changes. 
Therefore, it was erroneous for me to be clearing the clip flag when the UI
calls CurrentAudibleLevel().

Overall, I think it makes sense to simply maintain the power and clip state
per-stream and have the CurrentAudibleLevel() call scan all streams' power
levels.

https://codereview.chromium.org/14600025/diff/53001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/14600025/diff/53001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:249: #ifndef NDEBUG
On 2013/07/02 22:26:34, DaleCurtis wrote:
> As nice as it looks, you should probably remove prior to commit.

Done.

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:22: static const float kReferenceLevel =
1.0f;
On 2013/07/02 22:26:34, DaleCurtis wrote:
> enum instead?

Removed it.  I originally had this to make the calculations clearer on line 94,
but now I'm thinking this is just overkill.

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:24: const float
AudioPowerMonitor::kZeroPowerDBFS =
On 2013/07/02 22:26:34, DaleCurtis wrote:
> Hmm, I'm think this is a static initializer.

Replaced these float constants with inline static functions.  I did a bit of
digging around, and it seems that in C++03, the intention is for
numeric_limits<> functions to return constant expressions, though the language
cannot enforce this.  In C++11, the language and spec do enforce this.  Either
way, my solution here should prevent the compiler from generating initializers
and/or function calls in optimized code.

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:27: const float
AudioPowerMonitor::kMaxPowerDBFS = 0.0f;
On 2013/07/02 22:26:34, DaleCurtis wrote:
> enum?

See above comment.  However, I'd be a little wary of declaring these values
using enums for a few reasons.  Mostly, I'd worry that someone unintentionally
produces buggy code because they forget that using the constants won't promote
an integer value to a float type.

Off topic, but there was a time where we were playing around with the idea of a
dBFS data type (akin to how base::Time wraps int64), if it could be shown that
this would improve code quality.  For now, I don't see enough use cases yet to
warrant its development.

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:91: const float power = (average_power_ <
1.0e-10f) ? 0.0f : average_power_;
On 2013/07/02 22:26:34, DaleCurtis wrote:
> Should probably be a top level const.

Good catch.  I meant to break that out.  I kept it as a local const variable,
though, since it's only used in this one place.

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
File media/audio/audio_power_monitor_unittest.cc (right):

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor_unittest.cc:239: 0.0f, 0.0f, 0.0f,
std::numeric_limits<float>::infinity()
On 2013/07/02 22:26:34, DaleCurtis wrote:
> Static initializer.

Done.

https://codereview.chromium.org/14600025/diff/53001/media/audio/audio_power_m...
media/audio/audio_power_monitor_unittest.cc:243: 0.5f, -0.5f, 0.5f,
std::numeric_limits<float>::quiet_NaN()
On 2013/07/02 22:26:34, DaleCurtis wrote:
> Ditto.

Done.

[miu, 2013-07-09 01:14:56]

avi: Please review change to content/public/browser/media_observer.h

Thanks,
Yuri

[DaleCurtis, 2013-07-09 01:36:11]

lgtm (again, math deferred to crogers).

https://codereview.chromium.org/14600025/diff/63001/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.cc (right):

https://codereview.chromium.org/14600025/diff/63001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.cc:93: StreamPowerLevels::iterator
stream_it;
You could save some complexity with an std::find_if() here and below.

[miu, 2013-07-09 20:59:53]

https://codereview.chromium.org/14600025/diff/63001/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.cc (right):

https://codereview.chromium.org/14600025/diff/63001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.cc:93: StreamPowerLevels::iterator
stream_it;
On 2013/07/09 01:36:12, DaleCurtis wrote:
> You could save some complexity with an std::find_if() here and below.

Yeah, the minor code duplication going on here bothered me a little too.  I had
thought about this and tried it.  It's counter-intuitive, but it actually turns
out to be more complex: You save a few LOC within this method, but then have to
pull in two additional STL includes (<algorithm> and <functional>) and write a
helper method (the matcher).  If only we could use C++11 lambdas...  ;-)

[Avi (use Gerrit), 2013-07-16 23:08:18]

LGTM stamp for content.

[Chris Rogers, 2013-07-18 00:09:36]

Yuri, sorry for the long delay.

I'm restricting my review to the audio_power_monitor files.  Overall looks
pretty good, just some minor nits and comments.

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_output_...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_output_...
media/audio/audio_output_controller.h:55: class AudioPowerMonitor;
sss

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:60: const float sample_squared = (*p) * (*p);
it's inefficient to dereference *p twice, better to have:

const float sample = *p;
const float sample_squared = sample * sample;

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:81: const float kInsignificantPower =
1.0e-10f;
at your discretion: this is pretty small (-200dB) -- maybe could be a little
higher.  Just as a note, the lowest level signal in 16bit audio (1 bit) is
around -90dB

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
File media/audio/audio_power_monitor.h (right):

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:17: // low-pass filter over the samples
scanned.  Whenever reporting the power
"over the samples" -> "over the square of the samples"

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:39: // since the last invocation.
I think to really do this well, the idea of "clipped" needs to be maintained
over a longer time perioed than just every single invocation.  In UIs for peak
detectors (to detect "overs" above 0dbFS) there's usually a hold/timeout period
of a second or two if even one single sample is detected to be over.  That way
the UI can register something sinister like a very obvious red line or bar which
is persistent for a little while.

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:49: // callback will be posted.
Not that you have to change this all now, so I'll make this comment as just a
future suggestion.

I don't understand why we're doing the analysis and "pushing" the information to
another thread where the UI is drawn.  In other words, the |measurement_period|
and Post should not really be necessary.  In my experience with desktop/native
audio apps, normally this type of system simply is continuously analysing the
signal in the native audio thread and writing the power and peak value to a
memory location, which is then read by the UI thread which re-draws the metering
UI every 60 or 30 frames per seconds (or whatever the refresh rate), but the
timer triggering re-draw of the UI would be decoupled from this code here.

It seems overly complex to have to communicate with a Post, although I do
appreciate the effort put into making this safe using a "try" version of Post,
so I'm not adamently opposed to it...

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:67: 
Just for completeness, you might consider adding a Reset() method which re-inits
power state variables:
average_power_
clipped_since_last_notification_
frames_since_last_notification_
last_reported_power_
last_reported_clipped_

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:80: const PowerMeasurementCallback
notify_power_level_;
maybe a better name would be "power_level_callback_"

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:88: // notifications.
nit: notifications can go on previous comment line

[miu, 2013-07-19 01:08:07]

Thanks, Chris.  I've made some code changes and responded to your concerns.

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:60: const float sample_squared = (*p) * (*p);
On 2013/07/18 00:09:36, Chris Rogers wrote:
> it's inefficient to dereference *p twice, better to have:
> 
> const float sample = *p;
> const float sample_squared = sample * sample;

Done.  Though, wouldn't the compiler optimize that the same way?

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:81: const float kInsignificantPower =
1.0e-10f;
On 2013/07/18 00:09:36, Chris Rogers wrote:
> at your discretion: this is pretty small (-200dB) -- maybe could be a little
> higher.  Just as a note, the lowest level signal in 16bit audio (1 bit) is
> around -90dB

It's in terms of amplitude^2; so, it translates to -100dB.

Yes, I had assumed signed 16-bit PCM, and 2^-15 is approximately 3 * 10^-5.  So,
I put in 1 * 10^-5 as a threshold, and confirmed via testing that this worked
well.

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
File media/audio/audio_power_monitor.h (right):

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:17: // low-pass filter over the samples
scanned.  Whenever reporting the power
On 2013/07/18 00:09:36, Chris Rogers wrote:
> "over the samples" -> "over the square of the samples"

Done.

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:39: // since the last invocation.
On 2013/07/18 00:09:36, Chris Rogers wrote:
> I think to really do this well, the idea of "clipped" needs to be maintained
> over a longer time perioed than just every single invocation.  In UIs for peak
> detectors (to detect "overs" above 0dbFS) there's usually a hold/timeout
period
> of a second or two if even one single sample is detected to be over.  That way
> the UI can register something sinister like a very obvious red line or bar
which
> is persistent for a little while.

Agreed (with the UI details).  I wanted to keep the logic of the power
measurement separate from the UI details.  The callback here simply reports
changes in power levels (and clip status), whereas the UI code will interpret
how the data should be communicated visually.  I'm working with sail@ on this,
and will point this out to him as he proceeds with UI development.

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:49: // callback will be posted.
On 2013/07/18 00:09:36, Chris Rogers wrote:
> I don't understand why we're doing the analysis and "pushing" the information
to
> another thread where the UI is drawn.  In other words, the
|measurement_period|

I'll discuss this further with the UI side of the team.  I think the immediate
answer to your question is that the UI design around the existing indicators
uses a push model, and so leveraging that allowed us to get things done faster. 
We should revisit these design decisions once all tab UI indicators reach a more
stable state, and the interactions with the tab strip rendering implementation
is better understood.

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:67: 
On 2013/07/18 00:09:36, Chris Rogers wrote:
> Just for completeness, you might consider adding a Reset() method which
re-inits
> power state variables:

The issue here would be around the threading.  I assume the Reset() method would
be called by a thread other than the native audio thread, which means that
somehow these values would have to be reset transactionally.  While it's
possible to do without locking, I don't think I should add that until there's a
need for it.

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:80: const PowerMeasurementCallback
notify_power_level_;
On 2013/07/18 00:09:36, Chris Rogers wrote:
> maybe a better name would be "power_level_callback_"

Done.

https://codereview.chromium.org/14600025/diff/85001/media/audio/audio_power_m...
media/audio/audio_power_monitor.h:88: // notifications.
On 2013/07/18 00:09:36, Chris Rogers wrote:
> nit: notifications can go on previous comment line

It's one character too long.  :(  I s/notifications/callbacks/ to fit.

[commit-bot: I haz the power, 2013-07-19 01:08:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/14600025/102002

[commit-bot: I haz the power, 2013-07-19 02:38:23]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-07-19 02:40:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/14600025/102002

[commit-bot: I haz the power, 2013-07-19 06:46:42]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[miu, 2013-07-20 01:43:11]

Dale: PTAL at the diff between Patch Set 6 and 7.  The CQ caught a subtle
shutdown race condition when the PPAPI browser tests ran.  This led me to make a
"propagation" of code changes starting in AOC:

1. I slightly modified how AOC::DoClose() works.  See the comment I added in
AOC::DoClose(), as it explains the problem and the solution.

2. As expected, the possibility of objects disappearing as thread hopping occurs
caused me to make a few minor changes in AudioRendererHost as well.

3. Changes to AudioRendererHost required some changes to
audio_renderer_host_unittest.cc.  In addition, somebody recently did an ugly
copy-and-paste of 35 LOC, which I rectified.

Thanks,
Yuri

[DaleCurtis, 2013-07-22 17:27:20]

https://codereview.chromium.org/14600025/diff/137001/media/audio/audio_output...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/14600025/diff/137001/media/audio/audio_output...
media/audio/audio_output_controller.cc:217: // The AudioOutputController is now
closed.  Until a playing stream was fully
Is it really necessary to send a zero during close?  Won't
AudioRenderHost::DeleteEntry() be called immediately after this?  Kind of stinks
to add an extra hop to every Close() call.

[miu, 2013-07-22 21:10:47]

https://codereview.chromium.org/14600025/diff/137001/media/audio/audio_output...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/14600025/diff/137001/media/audio/audio_output...
media/audio/audio_output_controller.cc:217: // The AudioOutputController is now
closed.  Until a playing stream was fully
On 2013/07/22 17:27:21, DaleCurtis wrote:
> Is it really necessary to send a zero during close?  Won't
> AudioRenderHost::DeleteEntry() be called immediately after this?  Kind of
stinks
> to add an extra hop to every Close() call.

You're right, this is unnecessary since the final call to MediaObserver will
notify the stream has stopped playing in AudioRendererHost::DeleteEntry(). 
However, it is still needed for DoPause(), to notify that things are silent. 
I've moved it there.

Eliminating the extra hop: Although this is only a "trampoline" within the
current thread (i.e., this is not a thread hop), it just occurred to me I can
make this all simpler by using a base::CancelableCallback.  WDYT (patch set 8)?

[DaleCurtis, 2013-07-22 21:36:01]

lgtm, but your diff looks messed up, I'm seeing changes to
base/cancelable_callback.h which I'm assuming shouldn't be there.

[miu, 2013-07-22 21:44:11]

On 2013/07/22 21:36:01, DaleCurtis wrote:
> lgtm, but your diff looks messed up, I'm seeing changes to
> base/cancelable_callback.h which I'm assuming shouldn't be there.

It's legit.  I needed to add a template for two-argument callbacks.  Only zero-
and one- argument callbacks are currently implemented there.  :-/

Will get a reviewer for that...

Thanks again for the review, Dale.

[miu, 2013-07-22 22:40:59]

Mark: Would you take a look at the addition I made to
base/cancelable_callback.h?  I needed to add a template for two-argument
callbacks.

Thanks,
Yuri

[Mark Mentovai, 2013-07-23 01:05:36]

LGTM

[commit-bot: I haz the power, 2013-07-23 02:09:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/14600025/155001

[commit-bot: I haz the power, 2013-07-23 05:24:48]

Change committed as 213042