Dynamically FIFO when OSX requests unexpected frame counts.

Dynamically FIFO when OSX requests unexpected frame counts.

Previously we would just return silence in this case.  However we
did not invoke the AudioSourceCallback which would leave clients
hanging.

Traces don't seem to indicate this is the cause of OSX audio
dropouts (the "AUHALStream::Render" traces are not even present
when the bug occurs).  However I'd like to remove all instances
where we're returning silence w/o triggering an error.

This change fixes an additional issue where we weren't calculating
the latency correctly.  Chrome expects the AudioBuffersState to be
calculated for all channels, while CoreAudio expects mBytesPerFrame
to be sizeof(Float32) for planar data.

BUG=160920
TEST=force buffer size increase/decrease, check playback is okay.

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

[scherkus (not reviewing), 2013-11-13 18:35:19]

re: "This change fixes an additional issue where we weren't calculating the
latency correctly."

is there a strong reason to include this particular fix in this CL? it makes it
harder to review and in the case we have to revert and/or bisect, it's more
useful to have fixes as separate commits

https://codereview.chromium.org/70903002/diff/60001/media/audio/mac/audio_auh...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/70903002/diff/60001/media/audio/mac/audio_auh...
media/audio/mac/audio_auhal_mac.cc:204: // since our pipeline can't handle frame
size changes.  Generally this is a
is this referring to media::Pipeline, or something else? it'd be good to clarify
/ use different terminology

also thinking long term ... is this something we _should_ support? I know
regular audio/video playback has no issues dealing w/ variable buffer sizes but
that might not be true for other types of applications.

https://codereview.chromium.org/70903002/diff/60001/media/audio/mac/audio_auh...
media/audio/mac/audio_auhal_mac.cc:210: audio_fifo_.reset(new AudioPullFifo(
OOC does using the fifo have a perf impact? (i.e., is there a reason why we
couldn't use it all the time instead of special-casing it?)

I'm wondering about situations where, say, this only happens a few times before
going back to being normal and now we're stuck using the FIFO.

https://codereview.chromium.org/70903002/diff/60001/media/audio/mac/audio_auh...
File media/audio/mac/audio_auhal_mac.h (right):

https://codereview.chromium.org/70903002/diff/60001/media/audio/mac/audio_auh...
media/audio/mac/audio_auhal_mac.h:27: #include "media/base/audio_pull_fifo.h"
fwd decl instead?

[DaleCurtis, 2013-11-13 20:06:23]

I'm loathe to split out CLs which touch the same code since our CQ is so
wretched these days. In this case, its not unreasonable to bundle since its a
single line and the new FIFO code touches the same piece.

https://codereview.chromium.org/70903002/diff/60001/media/audio/mac/audio_auh...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/70903002/diff/60001/media/audio/mac/audio_auh...
media/audio/mac/audio_auhal_mac.cc:204: // since our pipeline can't handle frame
size changes.  Generally this is a
On 2013/11/13 18:35:19, scherkus wrote:
> is this referring to media::Pipeline, or something else? it'd be good to
clarify
> / use different terminology
> 
> also thinking long term ... is this something we _should_ support? I know
> regular audio/video playback has no issues dealing w/ variable buffer sizes
but
> that might not be true for other types of applications.

Comment clarified.

The shared memory is sized to fit, so no audio clients are currently prepared to
handle variable buffer sizes. Resolving that has a number of issues; e.g. slow
shared memory creation, signaling around current buffer size, etc. I expect the
situation would be difficult for WebRTC and WebAudio without incurring a latency
penalty.

https://codereview.chromium.org/70903002/diff/60001/media/audio/mac/audio_auh...
media/audio/mac/audio_auhal_mac.cc:210: audio_fifo_.reset(new AudioPullFifo(
On 2013/11/13 18:35:19, scherkus wrote:
> OOC does using the fifo have a perf impact? (i.e., is there a reason why we
> couldn't use it all the time instead of special-casing it?)
> 
> I'm wondering about situations where, say, this only happens a few times
before
> going back to being normal and now we're stuck using the FIFO.

Yes it has a perf impact of at least 6.5% (see AudioConverter perf test) due to
the multiple memcpys involved. Even if the correct frame size returned it'd be
impossible to drop the FIFO, since some remainder of audio data would always be 
present.

https://codereview.chromium.org/70903002/diff/60001/media/audio/mac/audio_auh...
File media/audio/mac/audio_auhal_mac.h (right):

https://codereview.chromium.org/70903002/diff/60001/media/audio/mac/audio_auh...
media/audio/mac/audio_auhal_mac.h:27: #include "media/base/audio_pull_fifo.h"
On 2013/11/13 18:35:19, scherkus wrote:
> fwd decl instead?

Done.

[DaleCurtis, 2013-11-16 00:01:48]

Ping? This is still something we should land.

[DaleCurtis, 2013-11-19 20:57:18]

Ping x2.

[scherkus (not reviewing), 2013-11-19 21:05:16]

Sorry .. still catching up.

How long does this silence situation last for? Are we talking milliseconds,
seconds, minutes, indefinitely, etc...? The comment seems to indicate that a
device change is imminent, so it'd be nice to have less complexity if we don't
really need this.

[DaleCurtis, 2013-11-19 22:48:53]

From observation, average case is milliseconds.  I'd guess, based on the time of
other CoreAudio operations, that the worst case ranges from seconds to
indefinitely if a device notification is delayed or missed.

If a callback occurs before our device change finishes, this will manifest as a
glitch to the user.  I also think this is a small enough patch that the tiny
additional complexity is outweighed by the safety of not having code which short
circuits our entire playback loop.  I'd certainly sleep easier knowing it's not
there. :)

[scherkus (not reviewing), 2013-11-20 20:37:51]

lgtm, but some things to keep in mind:
  - we can't tell externally if the code path is being used or not when future
bug reports arrive
  - we have no real data on the worst case scenarios you describe
  - I personally feel it's OK to drop out audio while swapping devices (i.e.,
this isn't a case worth optimizing for)

(also ... if you say it doesn't fix bug 160920, perhaps we should use a
different BUG= ?)

[DaleCurtis, 2013-11-20 21:27:38]

I'll add a VLOG(1) for FIFO creation so we'll at least have some logging
information when its engaged.

I'm okay with dropping audio too, just not callbacks. My original patch did just
that, but then I realized we need the FIFO to keep the callback timing
consistent to avoid playback rate changes. At which point it's just easier to
output the data than zero it.

[commit-bot: I haz the power, 2013-11-20 21:32:16]

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

[commit-bot: I haz the power, 2013-11-21 01:56:17]

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

[commit-bot: I haz the power, 2013-11-21 02:38:01]

Change committed as 236378