Used native deinterleaved and float point format for the input streams.

Used native deinterleaved and float point format for the input streams.

If we call GetProperty of kAudioUnitProperty_StreamFormat before setting the format, the device will report kAudioFormatFlagsNativeFloatPacked | kLinearPCMFormatFlagIsNonInterleaved as the native format of the device, which is the same as the output.

This patch changes the format to use kAudioFormatFlagsNativeFloatPacked | kLinearPCMFormatFlagIsNonInterleaved to open the device, so that we will avoid format flipping back and forth. Hope this optimization will help increase the stability of the input audio on Mac.


BUG=404884
TEST=media_unittests && https://webrtc.googlecode.com/svn-history/r5497/trunk/samples/js/demos/html/pc1.html, https://www.google.com/intl/en/chrome/demos/speech.html

Committed: https://crrev.com/c72c70da1229625cb54ecf683a909f173043d5b1
Cr-Commit-Position: refs/heads/master@{#292151}

[no longer working on chromium, 2014-08-24 14:42:30]

Hi Dale, this CL makes the input code on Mac to use the native format:
kAudioFormatFlagsNativeFloatPacked | kLinearPCMFormatFlagIsNonInterleaved to
open the device, which is exactly what the output code is doing.

I think this change will avoid the format flipping back and forth.

Please review.

Thanks,
SX

[DaleCurtis, 2014-08-25 17:42:24]

I don't really follow why the input format is flipping back and forth, can you
elaborate further?

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:45: for (int i = 0; i < channels;
++i) {
{} not necessary.

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:75: format_.mFormatFlags =
Hmm, I thought you tried this before creating the AudioBlockFifo and found it
didn't work for input?  What changed since then?

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:82: format_.mBytesPerPacket =
format_.mBytesPerFrame * format_.mFramesPerPacket;
This is not the same setting as output?

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:88: audio_buffer_list_ =
static_cast<AudioBufferList*>(
You allocate an audio_buffer_list_, but I don't see it actually being used...
What am I missing?  In output, the buffer list is provided by the callback.

https://codereview.chromium.org/501823002/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo.cc (right):

https://codereview.chromium.org/501823002/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:73: // Deinterleave the content to the FIFO and
update the |write_pos_|.
Comment is no longer relevant.

https://codereview.chromium.org/501823002/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo_unittest.cc (right):

https://codereview.chromium.org/501823002/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo_unittest.cc:19: class AudioBlockFifoTestP : public
AudioBlockFifoTest,
AudioBlockFifoFormatTest ?  The P is not really helpful :)

[no longer working on chromium, 2014-08-26 09:39:52]

Dale, please take another look.

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:45: for (int i = 0; i < channels;
++i) {
On 2014/08/25 17:42:23, DaleCurtis wrote:
> {} not necessary.

Done.

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:75: format_.mFormatFlags =
On 2014/08/25 17:42:23, DaleCurtis wrote:
> Hmm, I thought you tried this before creating the AudioBlockFifo and found it
> didn't work for input?  What changed since then?

It is the |format_| + |audio_buffer_list_| failed the previous attempt.
Particularly, previously audio_buffer_list_ is a variable, it only allocates a
sizeof(UInt32) + sizeof(AudioBuffer) memory which does not work for stereo
recording, which badly confused me.
In this CL, I change it to be a pointer so that we can dynamically allocate
enough memory, then it works.

We need to set the correct |format_| and also allocate enough memory to
|audio_buffer_list_|, otherwise AudioUnitRender() will fail or crash.

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:82: format_.mBytesPerPacket =
format_.mBytesPerFrame * format_.mFramesPerPacket;
On 2014/08/25 17:42:23, DaleCurtis wrote:
> This is not the same setting as output?

Practically it is the same, since format_.mFramesPerPacket == 1, so
mBytesPerPacket is sizeof(Float32).

The output code is making use of the fact that format_.mFramesPerPacket == 1

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:88: audio_buffer_list_ =
static_cast<AudioBufferList*>(
On 2014/08/25 17:42:23, DaleCurtis wrote:
> You allocate an audio_buffer_list_, but I don't see it actually being used...
> What am I missing?  In output, the buffer list is provided by the callback.

It is used in AudioUnitRender(), which is the method to get audio data from the
device, we need to provide our own AudioBufferList* memory to the method.
AUAudioInputStream::InputProc() contains the details.

https://codereview.chromium.org/501823002/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo.cc (right):

https://codereview.chromium.org/501823002/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:73: // Deinterleave the content to the FIFO and
update the |write_pos_|.
On 2014/08/25 17:42:23, DaleCurtis wrote:
> Comment is no longer relevant.

Done.

https://codereview.chromium.org/501823002/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo_unittest.cc (right):

https://codereview.chromium.org/501823002/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo_unittest.cc:19: class AudioBlockFifoTestP : public
AudioBlockFifoTest,
On 2014/08/25 17:42:23, DaleCurtis wrote:
> AudioBlockFifoFormatTest ?  The P is not really helpful :)

Done.

[DaleCurtis, 2014-08-26 18:51:55]

lgtm % comments.

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:75: format_.mFormatFlags =
On 2014/08/26 09:39:52, xians1 wrote:
> On 2014/08/25 17:42:23, DaleCurtis wrote:
> > Hmm, I thought you tried this before creating the AudioBlockFifo and found
it
> > didn't work for input?  What changed since then?
> 
> It is the |format_| + |audio_buffer_list_| failed the previous attempt.
> Particularly, previously audio_buffer_list_ is a variable, it only allocates a
> sizeof(UInt32) + sizeof(AudioBuffer) memory which does not work for stereo
> recording, which badly confused me.
> In this CL, I change it to be a pointer so that we can dynamically allocate
> enough memory, then it works.
> 
> We need to set the correct |format_| and also allocate enough memory to
> |audio_buffer_list_|, otherwise AudioUnitRender() will fail or crash.

Ah I see, that's great that it works.  We should probably unify this code into a
auhal_common.cc class or something so it can be shared between the input and
output drivers.  I'm okay if you do that in a separate CL though.

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:82: format_.mBytesPerPacket =
format_.mBytesPerFrame * format_.mFramesPerPacket;
On 2014/08/26 09:39:52, xians1 wrote:
> On 2014/08/25 17:42:23, DaleCurtis wrote:
> > This is not the same setting as output?
> 
> Practically it is the same, since format_.mFramesPerPacket == 1, so
> mBytesPerPacket is sizeof(Float32).
> 
> The output code is making use of the fact that format_.mFramesPerPacket == 1

Acknowledged.

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:88: audio_buffer_list_ =
static_cast<AudioBufferList*>(
On 2014/08/26 09:39:52, xians1 wrote:
> On 2014/08/25 17:42:23, DaleCurtis wrote:
> > You allocate an audio_buffer_list_, but I don't see it actually being
used...
> > What am I missing?  In output, the buffer list is provided by the callback.
> 
> It is used in AudioUnitRender(), which is the method to get audio data from
the
> device, we need to provide our own AudioBufferList* memory to the method.
> AUAudioInputStream::InputProc() contains the details.

Ahh, searching for audio_buffer_list_ showed no use, but I see it uses the class
accessor for AudioUnitRender().

In any case, please make this a scoped_ptr instead of a raw allocation.

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:107: audio_data_buffer_.reset();
No need for any of this after converting to scoped_ptr?

https://codereview.chromium.org/501823002/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo.h (right):

https://codereview.chromium.org/501823002/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.h:30: 
Add comment?

[no longer working on chromium, 2014-08-27 12:22:10]

Thanks Dale, I have already addressed all your comments and did one more minor
improvement on avoiding wrapping the memory in each callback.

I am going to land it now, if you have more comments, I will address them in a
follow-up CL.

Thanks,
SX

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:75: format_.mFormatFlags =
On 2014/08/26 18:51:54, DaleCurtis wrote:
> On 2014/08/26 09:39:52, xians1 wrote:
> > On 2014/08/25 17:42:23, DaleCurtis wrote:
> > > Hmm, I thought you tried this before creating the AudioBlockFifo and found
> it
> > > didn't work for input?  What changed since then?
> > 
> > It is the |format_| + |audio_buffer_list_| failed the previous attempt.
> > Particularly, previously audio_buffer_list_ is a variable, it only allocates
a
> > sizeof(UInt32) + sizeof(AudioBuffer) memory which does not work for stereo
> > recording, which badly confused me.
> > In this CL, I change it to be a pointer so that we can dynamically allocate
> > enough memory, then it works.
> > 
> > We need to set the correct |format_| and also allocate enough memory to
> > |audio_buffer_list_|, otherwise AudioUnitRender() will fail or crash.
> 
> Ah I see, that's great that it works.  We should probably unify this code into
a
> auhal_common.cc class or something so it can be shared between the input and
> output drivers.  I'm okay if you do that in a separate CL though.

Ok, lets do it in a separate refactoring CL then.

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:88: audio_buffer_list_ =
static_cast<AudioBufferList*>(
On 2014/08/26 18:51:54, DaleCurtis wrote:
> On 2014/08/26 09:39:52, xians1 wrote:
> > On 2014/08/25 17:42:23, DaleCurtis wrote:
> > > You allocate an audio_buffer_list_, but I don't see it actually being
> used...
> > > What am I missing?  In output, the buffer list is provided by the
callback.
> > 
> > It is used in AudioUnitRender(), which is the method to get audio data from
> the
> > device, we need to provide our own AudioBufferList* memory to the method.
> > AUAudioInputStream::InputProc() contains the details.
> 
> Ahh, searching for audio_buffer_list_ showed no use, but I see it uses the
class
> accessor for AudioUnitRender().
> 
> In any case, please make this a scoped_ptr instead of a raw allocation.

Thanks for pointing out, I forgot scoped_ptr could specify deleter, done with
using scoped_ptr

https://codereview.chromium.org/501823002/diff/20001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:107: audio_data_buffer_.reset();
On 2014/08/26 18:51:54, DaleCurtis wrote:
> No need for any of this after converting to scoped_ptr?

Done.

[no longer working on chromium, 2014-08-27 12:22:14]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-08-27 12:22:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xians@chromium.org/501823002/80001

[commit-bot: I haz the power, 2014-08-27 14:09:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[commit-bot: I haz the power, 2014-08-27 14:44:53]

Committed patchset #5 (80001) as 1a9ce977642c7f2cc2e30d83757c42264f052f0b

[eroman, 2014-08-27 17:13:24]

eroman@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2014-08-27 17:13:24]

There are some failures on mac asan bot, could it be from this changelist?

http://build.chromium.org/p/chromium.memory/buildstatus?builder=Mac%20ASan%20...

[no longer working on chromium, 2014-08-27 17:19:33]

On 2014/08/27 17:13:24, eroman wrote:
> There are some failures on mac asan bot, could it be from this changelist?
> 
>
http://build.chromium.org/p/chromium.memory/buildstatus?builder=Mac%20ASan%20...

Let me take a look, I will revert if it is the case.

[no longer working on chromium, 2014-08-27 17:26:47]

A revert of this CL (patchset #5) has been created in
https://codereview.chromium.org/514773002/ by xians@chromium.org.

The reason for reverting is: It broke the mac asan bot,
http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%....

[commit-bot: I haz the power, 2014-09-10 02:51:39]

Patchset 5 (id:??) landed as
https://crrev.com/c72c70da1229625cb54ecf683a909f173043d5b1
Cr-Commit-Position: refs/heads/master@{#292151}