Prevent AudioLowLatencyInputMac from changing frame sizes.

Prevent AudioLowLatencyInputMac from changing frame sizes.

Otherwise we'll hit a CHECK() later since the shared memory isn't configured for the new frame size.

BUG=154352
TEST=mac build no longer crashes.


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

[DaleCurtis, 2012-10-08 20:42:20]

scherkus is out, thanks for the review vrk!

[DaleCurtis, 2012-10-08 20:51:51]

Chris: Can you do the first pass here?

[Chris Rogers, 2012-10-08 21:08:26]

https://codereview.chromium.org/11086009/diff/1/media/audio/mac/audio_low_lat...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

https://codereview.chromium.org/11086009/diff/1/media/audio/mac/audio_low_lat...
media/audio/mac/audio_low_latency_output_mac.cc:242: // Unfortunately
AUAudioInputStream and AUAudioOutputStream share the frame
nit: "frame frame"

https://codereview.chromium.org/11086009/diff/1/media/audio/mac/audio_low_lat...
media/audio/mac/audio_low_latency_output_mac.cc:252: // fixed, a CHECK() should
be added and the clipping + wall of text removed.
Don't we still want to check for (audio_bus_->frames()) != number_of_frames) 
and simply memset(audio_data, 0, ...? and return early here?

[DaleCurtis, 2012-10-08 21:17:34]

https://codereview.chromium.org/11086009/diff/1/media/audio/mac/audio_low_lat...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

https://codereview.chromium.org/11086009/diff/1/media/audio/mac/audio_low_lat...
media/audio/mac/audio_low_latency_output_mac.cc:242: // Unfortunately
AUAudioInputStream and AUAudioOutputStream share the frame
On 2012/10/08 21:08:26, Chris Rogers wrote:
> nit: "frame frame"

Done.

https://codereview.chromium.org/11086009/diff/1/media/audio/mac/audio_low_lat...
media/audio/mac/audio_low_latency_output_mac.cc:252: // fixed, a CHECK() should
be added and the clipping + wall of text removed.
On 2012/10/08 21:08:26, Chris Rogers wrote:
> Don't we still want to check for (audio_bus_->frames()) != number_of_frames) 
> and simply memset(audio_data, 0, ...? and return early here?

Things are already broken so it doesn't really matter, but I thought keeping the
existing clipping behavior would be simpler.  One results in glitched audio the
other in silence.

[Chris Rogers, 2012-10-08 21:29:57]

I just thought it would be best to avoid the OnMoreData() call completely in the
error case since the callback rate might be a lot more (or less) frequent than
expected.  It might cause problems in other code because of this unexpected
change in rate as opposed to simply not calling OnMoreData() at all.

https://codereview.chromium.org/11086009/diff/4001/media/audio/mac/audio_low_...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

https://codereview.chromium.org/11086009/diff/4001/media/audio/mac/audio_low_...
media/audio/mac/audio_low_latency_output_mac.cc:260: frames_filled,
format_.mBitsPerChannel / 8, audio_data);
It looks like the ToInterleaved() call might write too much data to |audio_data|
if audio_bus_->frames()) != number_of_frames

[DaleCurtis, 2012-10-08 21:48:04]

Good point, I've changed both the input and output devices to do nothing when an
invalid frame size is detected.

https://codereview.chromium.org/11086009/diff/4001/media/audio/mac/audio_low_...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

https://codereview.chromium.org/11086009/diff/4001/media/audio/mac/audio_low_...
media/audio/mac/audio_low_latency_output_mac.cc:260: frames_filled,
format_.mBitsPerChannel / 8, audio_data);
On 2012/10/08 21:29:57, Chris Rogers wrote:
> It looks like the ToInterleaved() call might write too much data to
|audio_data|
> if audio_bus_->frames()) != number_of_frames

It will only write frames_filled which is clipped above.

[Chris Rogers, 2012-10-08 22:14:31]

lgtm with minor nits

https://codereview.chromium.org/11086009/diff/9/media/audio/mac/audio_low_lat...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/11086009/diff/9/media/audio/mac/audio_low_lat...
media/audio/mac/audio_low_latency_input_mac.cc:469: // size set by
kAudioDevicePropertyBufferFrameSize above on a per process
nit: "process process"

https://codereview.chromium.org/11086009/diff/9/media/audio/mac/audio_low_lat...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

https://codereview.chromium.org/11086009/diff/9/media/audio/mac/audio_low_lat...
media/audio/mac/audio_low_latency_output_mac.cc:240: // process basis.  What
this means is that the |number_of_frames| value may be
nit: "process process"

[DaleCurtis, 2012-10-08 22:21:56]

vrk?

https://codereview.chromium.org/11086009/diff/9/media/audio/mac/audio_low_lat...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/11086009/diff/9/media/audio/mac/audio_low_lat...
media/audio/mac/audio_low_latency_input_mac.cc:469: // size set by
kAudioDevicePropertyBufferFrameSize above on a per process
On 2012/10/08 22:14:31, Chris Rogers wrote:
> nit: "process process"

Done.

https://codereview.chromium.org/11086009/diff/9/media/audio/mac/audio_low_lat...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

https://codereview.chromium.org/11086009/diff/9/media/audio/mac/audio_low_lat...
media/audio/mac/audio_low_latency_output_mac.cc:240: // process basis.  What
this means is that the |number_of_frames| value may be
On 2012/10/08 22:14:31, Chris Rogers wrote:
> nit: "process process"

Done.

[vrk (LEFT CHROMIUM), 2012-10-08 22:51:01]

rubberstamp lgtm

[commit-bot: I haz the power, 2012-10-08 23:10:37]

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

[commit-bot: I haz the power, 2012-10-09 01:12:05]

Change committed as 160759