Automatically fall back to non-low latency on open() failure.

Automatically fall back to non-low latency on open() failure.

Pretty simple fallback solution. There are certainly other ways
we could do this, but this is perhaps the most succinct.

AudioOutputResampler will automatically downgrade to using the
non-low latency path when OpenStream() fails.

Some tweaking may be necessary to figure out the best parameters
to use when a low latency stream fails.

BUG=148418
TEST=Force failed low latency stream creation. Flash still works.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156341

[DaleCurtis, 2012-09-10 17:59:31]

PTAL guys at this first cut; this is based on top of
http://codereview.chromium.org/10918098/.

Viet-Trung: Let me know if this is what you guys are looking for on the PPAPI
side and further what your thoughts are on choosing buffer sizes etc for when we
downgrade to the non-low latency path. I.e. if we're given a low latency buffer
size of 512 we probably don't want to carry that over to the non-low latency
path.

[scherkus (not reviewing), 2012-09-11 12:36:26]

Is AudioOutputResampler the best location for handling this situation? It feels
like it's doing too much.

Is there some higher-level object that can hook into the failed call to
OpenStream() + recreate a PCM_LINEAR output, bypassing AOR completely?

[Ami GONE FROM CHROMIUM, 2012-09-11 15:11:19]

drive-by
Can/should we have UMA stats on how often fallback is needed / not needed?

(adding a UMA stat is as easy as:
UMA_HISTOGRAM_BOOLEAN("Media.FellBackToNonLowLatencyAudioPath",
required_fallback);
and a corresponding edit to histogram.xml to add the new bool).

[DaleCurtis, 2012-09-11 15:34:06]

On 2012/09/11 15:11:19, Ami Fischman wrote:
> drive-by
> Can/should we have UMA stats on how often fallback is needed / not needed?
> 
> (adding a UMA stat is as easy as:
> UMA_HISTOGRAM_BOOLEAN("Media.FellBackToNonLowLatencyAudioPath",
> required_fallback);
> and a corresponding edit to histogram.xml to add the new bool).

Yes, that's the plan. I just sent out the first CL to add UMA to the standard
path. I'll do the same with this CL after I sort out it's new layout.

[DaleCurtis, 2012-09-12 11:11:25]

On 2012/09/11 15:34:06, DaleCurtis wrote:
> On 2012/09/11 15:11:19, Ami Fischman wrote:
> > drive-by
> > Can/should we have UMA stats on how often fallback is needed / not needed?
> > 
> > (adding a UMA stat is as easy as:
> > UMA_HISTOGRAM_BOOLEAN("Media.FellBackToNonLowLatencyAudioPath",
> > required_fallback);
> > and a corresponding edit to histogram.xml to add the new bool).
> 
> Yes, that's the plan. I just sent out the first CL to add UMA to the standard
> path. I'll do the same with this CL after I sort out it's new layout.

PTAL. Per offline discussions with scherkus we want the low latency path to
eventually be as resilient as the high latency path, so rather than plumbing an
entire flow for clients to re-request with a low latency buffer, it makes sense
to just automatic downgrade.

The new patch set adds tests and UMA statistics for all this.

[scherkus (not reviewing), 2012-09-12 13:45:14]

http://codereview.chromium.org/10909151/diff/7001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy_unittest.cc (right):

http://codereview.chromium.org/10909151/diff/7001/media/audio/audio_output_pr...
media/audio/audio_output_proxy_unittest.cc:490: virtual void
InitDispatcher(base::TimeDelta close_delay, bool low_latency) {
can you replace the bool with the enum itself?

in your tests it looks like "InitDispatcher(..., true)" but the true isn't as
descriptive as the LOW_LATENCY enum itself

http://codereview.chromium.org/10909151/diff/7001/media/audio/audio_output_re...
File media/audio/audio_output_resampler.cc (right):

http://codereview.chromium.org/10909151/diff/7001/media/audio/audio_output_re...
media/audio/audio_output_resampler.cc:76: Initialize(true);
instead of passing true to call RecordStats ... how about calling RecordStats()
yourself right here?

[DaleCurtis, 2012-09-12 14:16:50]

http://codereview.chromium.org/10909151/diff/7001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy_unittest.cc (right):

http://codereview.chromium.org/10909151/diff/7001/media/audio/audio_output_pr...
media/audio/audio_output_proxy_unittest.cc:490: virtual void
InitDispatcher(base::TimeDelta close_delay, bool low_latency) {
On 2012/09/12 13:45:15, scherkus wrote:
> can you replace the bool with the enum itself?
> 
> in your tests it looks like "InitDispatcher(..., true)" but the true isn't as
> descriptive as the LOW_LATENCY enum itself

Done.

http://codereview.chromium.org/10909151/diff/7001/media/audio/audio_output_re...
File media/audio/audio_output_resampler.cc (right):

http://codereview.chromium.org/10909151/diff/7001/media/audio/audio_output_re...
media/audio/audio_output_resampler.cc:76: Initialize(true);
On 2012/09/12 13:45:15, scherkus wrote:
> instead of passing true to call RecordStats ... how about calling
RecordStats()
> yourself right here?

Done.

[scherkus (not reviewing), 2012-09-12 14:24:47]

lgtm w/ q

http://codereview.chromium.org/10909151/diff/7002/media/audio/audio_output_re...
File media/audio/audio_output_resampler.cc (right):

http://codereview.chromium.org/10909151/diff/7002/media/audio/audio_output_re...
media/audio/audio_output_resampler.cc:75: output_params_(output_params) {
it it worth DCHECKing that we're in LOW_LATENCY or is passing in LINEAR
supported use case?

[DaleCurtis, 2012-09-12 14:32:33]

http://codereview.chromium.org/10909151/diff/7002/media/audio/audio_output_re...
File media/audio/audio_output_resampler.cc (right):

http://codereview.chromium.org/10909151/diff/7002/media/audio/audio_output_re...
media/audio/audio_output_resampler.cc:75: output_params_(output_params) {
On 2012/09/12 14:24:47, scherkus wrote:
> it it worth DCHECKing that we're in LOW_LATENCY or is passing in LINEAR
> supported use case?

No reason it wouldn't work, so I haven't felt the need to restrict it. There may
be some use case in the future where we want to use WaveOut +
Rebuffering/Resampling.

[commit-bot: I haz the power, 2012-09-12 15:04:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10909151/9008

[DaleCurtis, 2012-09-12 15:05:30]

Due to the time difference and since it's still behind a flag, landing this now.
Please continue to review and I'll fix any issues in a follow up patch set.

[commit-bot: I haz the power, 2012-09-12 15:15:44]

Failed to apply patch for media/audio/audio_output_resampler.cc:
While running patch -p1 --forward --force;
patching file media/audio/audio_output_resampler.cc
Hunk #1 FAILED at 9.
Hunk #2 FAILED at 25.
Hunk #3 succeeded at 25 (offset -40 lines).
Hunk #4 FAILED at 124.
Hunk #5 succeeded at 165 (offset -43 lines).
Hunk #6 succeeded at 199 (offset -43 lines).
3 out of 6 hunks FAILED -- saving rejects to file
media/audio/audio_output_resampler.cc.rej

[commit-bot: I haz the power, 2012-09-12 15:35:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10909151/10005

[commit-bot: I haz the power, 2012-09-12 16:14:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10909151/2011

[Chris Rogers, 2012-09-12 18:45:45]

https://chromiumcodereview.appspot.com/10909151/diff/2011/media/audio/audio_o...
File media/audio/audio_output_resampler.cc (right):

https://chromiumcodereview.appspot.com/10909151/diff/2011/media/audio/audio_o...
media/audio/audio_output_resampler.cc:57: output_params.channels(),
limits::kMaxChannels);
Any chance we can record the buffer-size - frames_per_buffer()?
This seems like an important bit of information to have.

[commit-bot: I haz the power, 2012-09-12 18:49:23]

Change committed as 156341

[scherkus (not reviewing), 2012-09-13 11:58:13]

On 2012/09/12 18:45:45, Chris Rogers wrote:
>
https://chromiumcodereview.appspot.com/10909151/diff/2011/media/audio/audio_o...
> File media/audio/audio_output_resampler.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10909151/diff/2011/media/audio/audio_o...
> media/audio/audio_output_resampler.cc:57: output_params.channels(),
> limits::kMaxChannels);
> Any chance we can record the buffer-size - frames_per_buffer()?
> This seems like an important bit of information to have.

Is that a subtraction sign or are you saying to record frames_per_buffer()?

Dale and I chatted about this last night -- the hardware buffer size is either
fixed or derived from the hardware sample rate, meaning we end up with a
histogram version of the constants encoded in GetAudioHardwareBufferSize()
(i.e., not very useful data!)