Refactor the AudioDevice for iOS and improve the performance and stability

Refactor the AudioDevice for iOS and improve the performance and stability

This CL contains major modifications of the audio output parts for WebRTC on iOS:
- general code cleanup
- improves thread handling (added thread checks, remove critical section, atomic ops etc.)
- reduces loopback latency of iPhone 6 from ~90ms to ~60ms ;-)
- improves selection of audio parameters on iOS
- reduces complexity by removing complex and redundant delay estimates
- now instead uses fixed delay estimates if for some reason the SW EAC must be used
- adds AudioFineBuffer to compensate for differences in native output buffer size and
  the 10ms size used by WebRTC. Same class as is used today on Android and we have unit tests for
  this class (the old code was buggy and we have several issue reports of crashes related to it)

Similar improvements will be done for the recording sid as well in a separate CL.
I will also add support for 48kHz in an upcoming CL since that will improve Opus performance.

BUG=webrtc:4796, webrtc:4817, webrtc:4954, webrtc:4212
TEST=AppRTC demo and iOS modules_unittests using --gtest_filter=AudioDevice*
R=pbos@webrtc.org, tkchin@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/86d907cffda803ee34ee68f9833c1980d1b9f7a6

[henrika_webrtc, 2015-08-25 14:52:57]

henrika@webrtc.org changed reviewers:
+ tkchin@webrtc.org

[henrika_webrtc, 2015-08-25 14:55:49]

tkchin: hope you like this one. Many thanks for reviewing ;-)

[henrika_webrtc, 2015-08-26 16:09:25]

FYI, I was actually able to add support for a new recording structure (without
the extra thread) and also
support for 48kHz today. Please allow me to add that to this CL as well. Then
all parts can be reviewed
in one step. I will ping when done.

[tkchin_webrtc, 2015-08-26 23:26:52]

On 2015/08/26 16:09:25, henrika_webrtc wrote:
> FYI, I was actually able to add support for a new recording structure (without
> the extra thread) and also
> support for 48kHz today. Please allow me to add that to this CL as well. Then
> all parts can be reviewed
> in one step. I will ping when done.

Ok, sounds good to me!

[henrika_webrtc, 2015-08-28 10:40:11]

Patchset #2 (id:20001) has been deleted

[henrika_webrtc, 2015-08-28 10:40:31]

Patchset #2 (id:20001) has been deleted

[henrika_webrtc, 2015-08-28 10:40:41]

Patchset #6 (id:120001) has been deleted

[henrika_webrtc, 2015-08-28 14:07:07]

Should be OK now. Please review ;-)

See https://code.google.com/p/webrtc/issues/detail?id=4954 for more details.

[tkchin_webrtc, 2015-09-01 20:54:51]

Mostly nits, and a couple of n00b questions.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/fine_audio_buffer.cc (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/fine_audio_buffer.cc:32: cached_bytes_(0),
nit: consider naming this playout_cached_bytes_

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/fine_audio_buffer.cc:34: cache_buffer_.reset(new
int8_t[bytes_per_10_ms_]);
nit: consider naming this playout_cache_buffer_ to match record_cache_buffer_

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/fine_audio_buffer.cc:36: new
int8_t[desired_frame_size_bytes + bytes_per_10_ms_]);
nit: pull out size into a local var since it's the same in two places, can call
RequiredPlayoutBufferSizeBytes() too if that makes sense

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/fine_audio_buffer.cc:127:
memmove(record_cache_buffer_.get(),
Isn't this expensive? Performs a copy of all remaining bytes each iteration. Or
are the sizes small enough that it doesn't matter?

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/fine_audio_buffer.h (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/fine_audio_buffer.h:81: // Sample rete in Hertz.
nit: rate

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/include/audio_device_defines.h (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/include/audio_device_defines.h:164: void reset(int
sample_rate, int channels, double milliseconds_per_buffer) {
nit: ms_per_buffer is prob ok

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/include/audio_device_defines.h:169:
reset(sample_rate, channels, static_cast<size_t>(0));
nit: 0u?

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/include/audio_device_defines.h:186: bool
is_complete() const { return (is_valid() && (frames_per_buffer_ > 0)); }
When is it useful to be valid but incomplete?

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_ios.h (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:67: // return fixed
(reasonable) values here instead. Any AEC in WebRTC will then
ooc, what happens if two aec passes occur? Does it essentially no-op?

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:174: static OSStatus
RecordedDataIsAvailable(
nit: Style guide is to use C++ syntax/style in C++ classes.
So use in_ref_con etc.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:201: rtc::ThreadChecker
_threadChecker;
nit: Style guide is to use C++ syntax/style in C++ classes.
So these should be threadChecker_; etc

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:236: // cach non-utilized
audio between callbacks.
Thanks for this comment - it was helpful.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:236: // cach non-utilized
audio between callbacks.
nit: cache non-utilized

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_ios.mm (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:44: // in the I/O audio
unit. Seems to work only for built-in microphones.
Did you try this with bluetooth microphone? Wondering how you found out it only
works for built-in mic.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:388: void
AudioDeviceIOS::VerifyAudioParametersForActiveAudioSession() {
This does a bit more than verifying parameters, consider naming it something
like SetupAudioBuffersForActiveAudioSession

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:391: // Verify the current
values once the audio session has been activated.
minor minor nit: line break after comment + block for comment to make things
easier to read in this function

e.g.
// comment about XYZ
XYZ

// comment about PQR
PQR

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:400:
CHECK_EQ(static_cast<int>(session.sampleRate),
It seems overkill to crash if we don't get what we need. It seems better to just
have no audio. (Video may still be usable).

Leaving as TODO is fine though.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:465:
CHECK_EQ(AudioComponentInstanceNew(foundVpioUnitRef, &_vpioUnit), noErr)
ditto system call failures shouldn't crash applications, here and below

probably return bool

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:645: AudioDeviceIOS* thiz =
static_cast<AudioDeviceIOS*>(inRefCon);
nit: would just call it audio_device_ios instead of thiz

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:661: // Since it happens on
he input bus, the |ioData| parameter is a reference
nit: the input bus

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:668: LOG_F(LS_ERROR) <<
"AudioOutputUnitStart failed: " << result;
Is this only called during start? I read it as a callback that happens each time
there is data.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:677:
_fineAudioBuffer->DeliverRecordedData(data, dataSizeInBytes,
What threading requirements are there for DeliverRecordedData? Is there an
eventual thread dispatch / copy to something else for encoding?

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:693: return
thiz->OnGetPlayoutData(ioActionFlags, inNumberFrames, ioData);
ditto audio_device_ios

[henrika_webrtc, 2015-09-03 13:44:42]

Thanks!

Done most of the changes but there might be some parts left. I have not changed
the naming scheme to C++ yet (see comments).

Could you please check again and I'll try to finalize tomorrow ;-)

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/fine_audio_buffer.cc (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/fine_audio_buffer.cc:32: cached_bytes_(0),
On 2015/09/01 20:54:49, tkchin_webrtc wrote:
> nit: consider naming this playout_cached_bytes_

Done.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/fine_audio_buffer.cc:34: cache_buffer_.reset(new
int8_t[bytes_per_10_ms_]);
On 2015/09/01 20:54:49, tkchin_webrtc wrote:
> nit: consider naming this playout_cache_buffer_ to match record_cache_buffer_

Done.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/fine_audio_buffer.cc:36: new
int8_t[desired_frame_size_bytes + bytes_per_10_ms_]);
Thanks. Done.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/fine_audio_buffer.cc:127:
memmove(record_cache_buffer_.get(),
I have modified the existing scheme by extending the size of the temporary
buffer with a factor of 5.
On my iPhone 6, it results in a total cache size of 9920 bytes but now we only
do a memmove() once every tenth call, i.e.,
store & send
... 7 more
store & send 
memmove()
store & send
... 7 more
store & send
ec.
and the largest block we move is 896 bytes (smallest is 64)

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/fine_audio_buffer.h (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/fine_audio_buffer.h:81: // Sample rete in Hertz.
On 2015/09/01 20:54:49, tkchin_webrtc wrote:
> nit: rate

Done.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/include/audio_device_defines.h (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/include/audio_device_defines.h:164: void reset(int
sample_rate, int channels, double milliseconds_per_buffer) {
On 2015/09/01 20:54:50, tkchin_webrtc wrote:
> nit: ms_per_buffer is prob ok

Done.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/include/audio_device_defines.h:169:
reset(sample_rate, channels, static_cast<size_t>(0));
pkasting just landed this static_cast in a CL where he cleaned up how we use
size_t. Hope it is OK if I keep it as is since it was his choice to use this
method.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/include/audio_device_defines.h:186: bool
is_complete() const { return (is_valid() && (frames_per_buffer_ > 0)); }
Good question. Perhaps the current notation is not perfect but with a set that
is valid, you can call AudioDeviceIOS::UpdateAudioDeviceBuffer() which informs
the upper layers (WebRTC running at 10ms) about the sample rate and the number
of channels. In this state one can start to ask for audio from WebRTC and to
send recorded data to it.
But, the AudioParameters struct is not yet complete if it does not contain the
internal/native buffer size as well. This parts is only used by the lower layers
but I found it handy to add an extra check for it. Hence, 'complete' means
'valid for WebRTC communication' and possibility to kick of the native parts as
well.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_ios.h (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:67: // return fixed
(reasonable) values here instead. Any AEC in WebRTC will then
Extended the comments. Today we always disable all internal (WebRTC) AEC and
will only run the built-in version. These APIs will still be called but for
other reasons (A/V sync) and we avoid spamming the log with error messages by
returning sane values here. The result will be ignored.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:174: static OSStatus
RecordedDataIsAvailable(
ooh nooo, I was not sure if one could use objc notation here or not...

So, basically, you are asking me to change the style in the complete class.
Personally, I like the existing style but I hear you :-)

And there is no chance that you are OK as is...guess not.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:201: rtc::ThreadChecker
_threadChecker;
On 2015/09/01 20:54:50, tkchin_webrtc wrote:
> nit: Style guide is to use C++ syntax/style in C++ classes.
> So these should be threadChecker_; etc

Acknowledged.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:236: // cach non-utilized
audio between callbacks.
Thanks. Added comments for the recording side as well.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:236: // cach non-utilized
audio between callbacks.
On 2015/09/01 20:54:50, tkchin_webrtc wrote:
> nit: cache non-utilized

Done.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_ios.mm (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:44: // in the I/O audio
unit. Seems to work only for built-in microphones.
I'll see if I can test with a BT device as well. My test shows that you can't
ask for mono and get mono as native format on the built-in mic. When we plug in
a mic, we always get stereo as native input but will be converted in the format
converted in the audio unit to match mono. Perhaps the comment is badly written.

OK, I was able to connect a Plantronics BT device and it also supports native
mono input (same as built-in).

However, we will crash today since it does not support 48kHz _only_ 8kHz (hence,
the existing code does not work either since it tries to open in 16kHz).

I will add a TODO to fix BT support in an upcoming CL. Hope that is OK. I have
verified that it works in loopback with a BT device using this code, but we will
fail when we check that we got the sample rate that was asked for. I will redo
this scheme and add a fallback but would like to do that in a separate CL.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:388: void
AudioDeviceIOS::VerifyAudioParametersForActiveAudioSession() {
On 2015/09/01 20:54:50, tkchin_webrtc wrote:
> This does a bit more than verifying parameters, consider naming it something
> like SetupAudioBuffersForActiveAudioSession

Done.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:391: // Verify the current
values once the audio session has been activated.
On 2015/09/01 20:54:50, tkchin_webrtc wrote:
> minor minor nit: line break after comment + block for comment to make things
> easier to read in this function
> 
> e.g.
> // comment about XYZ
> XYZ
> 
> // comment about PQR
> PQR

Done.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:400:
CHECK_EQ(static_cast<int>(session.sampleRate),
Agree. Let me keep the TODO and fix this in combination with adding support for
BT. I'll try to ensure that we fall back to the rate that we got. I think it
will be possible with the new structure. I.e., audio should work even when 48 is
not supported.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:465:
CHECK_EQ(AudioComponentInstanceNew(foundVpioUnitRef, &_vpioUnit), noErr)
OK; I will fix that. Must leave soon but will fix it tomorrow.

And, I assume you want me to add new-line before new comments here as well.
Correct?

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:645: AudioDeviceIOS* thiz =
static_cast<AudioDeviceIOS*>(inRefCon);
On 2015/09/01 20:54:50, tkchin_webrtc wrote:
> nit: would just call it audio_device_ios instead of thiz

Done.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:661: // Since it happens on
he input bus, the |ioData| parameter is a reference
On 2015/09/01 20:54:50, tkchin_webrtc wrote:
> nit: the input bus

Done.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:668: LOG_F(LS_ERROR) <<
"AudioOutputUnitStart failed: " << result;
It is not a callback on the capture side. The callback is
RecordedDataIsAvailable() and once we are there we can ask for the actual
recorded data using AudioUnitRender(). It feels as if the scheme is written for
playout only initially and when recording was added, the naming did not become
perfect. But no, it is not a callback.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:677:
_fineAudioBuffer->DeliverRecordedData(data, dataSizeInBytes,
The data is copied to an internal buffer inside the platform independent code in
AudioDeviceBuffer. FineAudioBuffer will ensure that this copy is sent to WebRTC
on this thread (the high-prio IO thread). I must admit that I don't know the
exact details of the latest APM version and the threading model it uses, but I
assume it has a separate encoding and sending thread. I must ask around to
improve this feedback.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:693: return
thiz->OnGetPlayoutData(ioActionFlags, inNumberFrames, ioData);
On 2015/09/01 20:54:50, tkchin_webrtc wrote:
> ditto audio_device_ios

Done.

[tkchin_webrtc, 2015-09-04 05:29:16]

lgtm

At the risk of sounding repetitive, please remove CHECK bits soon :) But I
understand that this is WIP.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_ios.h (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:174: static OSStatus
RecordedDataIsAvailable(
On 2015/09/03 13:44:41, henrika_webrtc wrote:
> ooh nooo, I was not sure if one could use objc notation here or not...
> 
> So, basically, you are asking me to change the style in the complete class.
> Personally, I like the existing style but I hear you :-)
> 
> And there is no chance that you are OK as is...guess not.

I'm ok with changing old stuff separately not part of this CL. But the new stuff
you're adding should conform to C++ style. If you'd like to do that separately
too I'm also okay with that, I don't want to block this CL on that.

I'd suggest a single future CL that only does style changes without any
functionality chages, done after the the CL for removing crash behavior.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_ios.mm (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:44: // in the I/O audio
unit. Seems to work only for built-in microphones.
On 2015/09/03 13:44:41, henrika_webrtc wrote:
> I'll see if I can test with a BT device as well. My test shows that you can't
> ask for mono and get mono as native format on the built-in mic. When we plug
in
> a mic, we always get stereo as native input but will be converted in the
format
> converted in the audio unit to match mono. Perhaps the comment is badly
written.
> 
> OK, I was able to connect a Plantronics BT device and it also supports native
> mono input (same as built-in).
> 
> However, we will crash today since it does not support 48kHz _only_ 8kHz
(hence,
> the existing code does not work either since it tries to open in 16kHz).
> 
> I will add a TODO to fix BT support in an upcoming CL. Hope that is OK. I have
> verified that it works in loopback with a BT device using this code, but we
will
> fail when we check that we got the sample rate that was asked for. I will redo
> this scheme and add a fallback but would like to do that in a separate CL.

That's fine since this is active work. We should finish it before next branch
cut though since I think some external people take webrtc cuts then and we don't
want to break everyone. At the very least we shouldn't crash on assert.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:400:
CHECK_EQ(static_cast<int>(session.sampleRate),
On 2015/09/03 13:44:41, henrika_webrtc wrote:
> Agree. Let me keep the TODO and fix this in combination with adding support
for
> BT. I'll try to ensure that we fall back to the rate that we got. I think it
> will be possible with the new structure. I.e., audio should work even when 48
is
> not supported. 

For next CL, I would just have a simple one that removes the crashing behavior.
If it's easy to add the BT stuff to it too then sure, but let's get the
non-crashing behavior back in ASAP.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:668: LOG_F(LS_ERROR) <<
"AudioOutputUnitStart failed: " << result;
On 2015/09/03 13:44:41, henrika_webrtc wrote:
> It is not a callback on the capture side. The callback is
> RecordedDataIsAvailable() and once we are there we can ask for the actual
> recorded data using AudioUnitRender(). It feels as if the scheme is written
for
> playout only initially and when recording was added, the naming did not become
> perfect. But no, it is not a callback.

Ah I gotcha.

[henrika_webrtc, 2015-09-04 09:51:18]

Comments only. Will make one more round where I avoid crash on assert.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_ios.h (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:174: static OSStatus
RecordedDataIsAvailable(
Agree. And thanks for letting me do this in a separate CL.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_ios.mm (right):

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:44: // in the I/O audio
unit. Seems to work only for built-in microphones.
Will avoid crash on assert.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:400:
CHECK_EQ(static_cast<int>(session.sampleRate),
I'll add the non-crashing parts to this CL and BT support if I can make it
simple. If not, add in a follow-up.

https://codereview.webrtc.org/1254883002/diff/260001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:668: LOG_F(LS_ERROR) <<
"AudioOutputUnitStart failed: " << result;
On 2015/09/04 05:29:16, tkchin_webrtc wrote:
> On 2015/09/03 13:44:41, henrika_webrtc wrote:
> > It is not a callback on the capture side. The callback is
> > RecordedDataIsAvailable() and once we are there we can ask for the actual
> > recorded data using AudioUnitRender(). It feels as if the scheme is written
> for
> > playout only initially and when recording was added, the naming did not
become
> > perfect. But no, it is not a callback.
> 
> Ah I gotcha.

Acknowledged.

[henrika_webrtc, 2015-09-04 14:43:44]

The current version no longer crashes when it hits an assert and I have also
ensured that we
can use a non-preferred sample rate (i.e. BT headset does work).

Feel free to comment one last time on my latest changes.

I would like to run some CPU measurements as well before I land. Will finalize
on Monday.

Thanks!

[tkchin_webrtc, 2015-09-04 22:33:05]

Really looking forward to this commit. Just one copyright issue now.

https://codereview.webrtc.org/1254883002/diff/300001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_ios.mm (right):

https://codereview.webrtc.org/1254883002/diff/300001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:70: // This utility class is
taken from PublicUtility/CAXException.h.
I don't think we can do this. Can't copy code from copyrighted Apple code and
paste it into BSD WebRTC code.

If you want to use it, use the class directly by importing the header.

[henrika_webrtc, 2015-09-07 12:42:17]

https://codereview.webrtc.org/1254883002/diff/300001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_ios.mm (right):

https://codereview.webrtc.org/1254883002/diff/300001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:70: // This utility class is
taken from PublicUtility/CAXException.h.
Thanks. Now removed.

[henrika_webrtc, 2015-09-07 12:42:27]

The CQ bit was checked by henrika@webrtc.org

[henrika_webrtc, 2015-09-07 12:42:27]

The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1254883002/#ps320001 (title:
"Rebased and cleaned up")

[commit-bot: I haz the power, 2015-09-07 12:42:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254883002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254883002/320001

[commit-bot: I haz the power, 2015-09-07 12:49:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-07 12:49:05]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/634)

[henrika_webrtc, 2015-09-07 12:56:48]

henrika@webrtc.org changed reviewers:
+ pbos@webrtc.org

[henrika_webrtc, 2015-09-07 12:57:53]

+pbos: need your OK for talk/media/webrtc/webrtcvoiceengine.cc. Only trivial
logging changes. Please check. Thanks :-)

[pbos-webrtc, 2015-09-07 12:59:26]

lgtm for talk/

[henrika_webrtc, 2015-09-07 12:59:54]

The CQ bit was checked by henrika@webrtc.org

[commit-bot: I haz the power, 2015-09-07 12:59:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254883002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254883002/320001

[commit-bot: I haz the power, 2015-09-07 13:06:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-07 13:06:49]

Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[henrika_webrtc, 2015-09-07 14:10:17]

Committed patchset #15 (id:320001) manually as
86d907cffda803ee34ee68f9833c1980d1b9f7a6 (presubmit successful).