Provide isAudioEnabled flag to control audio unit.

Provide isAudioEnabled flag to control audio unit.

- Also removes async invoker usage in favor of thread posting

BUG=

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

[tkchin_webrtc, 2016-05-04 01:32:11]

Description was changed from

==========
Provide isAudioEnabled flag to control audio unit.

BUG=
==========

to

==========
Provide isAudioEnabled flag to control audio unit.

- Also removes async invoker usage in favor of thread posting
- Also responds to playback sample rate changes from audio unit

BUG=
==========

[tkchin_webrtc, 2016-05-04 01:35:07]

Patchset #1 (id:1) has been deleted

[tkchin_webrtc, 2016-05-04 01:35:36]

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

[tkchin_webrtc, 2016-05-04 02:37:55]

Patchset #1 (id:20001) has been deleted

[henrika_webrtc, 2016-05-04 12:32:57]

Looks really great. Took a quick initial look (half day here today). Will check
more on Friday (holiday tomorrow).

[henrika_webrtc, 2016-05-04 12:33:07]

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/ios/audio_device_ios.mm (right):

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/audio_device_ios.mm:440: void
AudioDeviceIOS::OnMessage(rtc::Message *msg) {
nice!

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/audio_device_ios.mm:534: // Mad hack: Sometimes
the audio session is already active because the app
Wow. Do you have a case where you can reliably test that this approach works?

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/ios/objc/RTCAudioSession.h (right):

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/objc/RTCAudioSession.h:104: /** Setting this to
YES will cause the audio session to be configured for WebRTC
Rather tricky to read IMHO. Can you add an example or a slightly more simple
comment as well which explains the usage from a higher level?

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm (right):

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:242: OSStatus
result =
Perhaps a comment regarding why we need this listener.

Apple states "Listen for this property when you do care what the hardware sample
rate is and you want to match your audio processing to the hardware sample rate
(this is the typical case).

The Audio Unit will send you a notification when the hardware sample rate
changes. Your application will need to stop rendering, reconfigure any
processing being done for the new sample rate, then start rendering again. The
sample rate change will be on the Output scope of Bus 0.

Your application should check the rate when called and only do any
reconfiguration when the new rate is different from the current rate. When the
sample rates differ, stop rendering, reconfiguring your audio processing to the
new sample rate and then restart."

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:273: result =
Do we need a check that it is disabled at close?

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:341: void
VoiceProcessingAudioUnit::OnSampleRateChange(
Can we now get this callback and the routing change callback?
Conflicts?

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:379: // We only
care about the output.
Have you really been able to trigger callback on other scope/bus?

[tkchin_webrtc, 2016-05-05 23:23:09]

Patchset #4 (id:100001) has been deleted

[tkchin_webrtc, 2016-05-05 23:23:28]

PTAL

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/ios/audio_device_ios.mm (right):

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/audio_device_ios.mm:440: void
AudioDeviceIOS::OnMessage(rtc::Message *msg) {
On 2016/05/04 12:33:07, henrika_webrtc wrote:
> nice!

Acknowledged.

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/audio_device_ios.mm:534: // Mad hack: Sometimes
the audio session is already active because the app
On 2016/05/04 12:33:07, henrika_webrtc wrote:
> Wow. Do you have a case where you can reliably test that this approach works?

Yup. With my BT headset. I'm backing out the change for now though to
investigate other approaches separately. I'd rather get the rest of this in
first to address crashes.

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/ios/objc/RTCAudioSession.h (right):

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/objc/RTCAudioSession.h:104: /** Setting this to
YES will cause the audio session to be configured for WebRTC
On 2016/05/04 12:33:07, henrika_webrtc wrote:
> Rather tricky to read IMHO. Can you add an example or a slightly more simple
> comment as well which explains the usage from a higher level?

Better?

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm (right):

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:242: OSStatus
result =
On 2016/05/04 12:33:07, henrika_webrtc wrote:
> Perhaps a comment regarding why we need this listener.
> 
> Apple states "Listen for this property when you do care what the hardware
sample
> rate is and you want to match your audio processing to the hardware sample
rate
> (this is the typical case).
> 
> The Audio Unit will send you a notification when the hardware sample rate
> changes. Your application will need to stop rendering, reconfigure any
> processing being done for the new sample rate, then start rendering again. The
> sample rate change will be on the Output scope of Bus 0.
> 
> Your application should check the rate when called and only do any
> reconfiguration when the new rate is different from the current rate. When the
> sample rates differ, stop rendering, reconfiguring your audio processing to
the
> new sample rate and then restart."

Removing these changes for now. Will address BT issues separately.

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:273: result =
On 2016/05/04 12:33:07, henrika_webrtc wrote:
> Do we need a check that it is disabled at close?

Removing these changes for now. Will address BT issues separately.

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:341: void
VoiceProcessingAudioUnit::OnSampleRateChange(
On 2016/05/04 12:33:07, henrika_webrtc wrote:
> Can we now get this callback and the routing change callback?
> Conflicts?

There shouldn't be conflicts - we are always using the sample rate from the
audio session.

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:379: // We only
care about the output.
On 2016/05/04 12:33:07, henrika_webrtc wrote:
> Have you really been able to trigger callback on other scope/bus?

Nope. But just in case.

[henrika_webrtc, 2016-05-06 11:22:17]

I think we should land this and take it from there. I can give it a test drive
in AppRTCDemo then as well.

Zeke: what should I then focus on testing?

LGTM

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/ios/audio_device_ios.mm (right):

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/audio_device_ios.mm:534: // Mad hack: Sometimes
the audio session is already active because the app
On 2016/05/05 23:23:27, tkchin_webrtc wrote:
> On 2016/05/04 12:33:07, henrika_webrtc wrote:
> > Wow. Do you have a case where you can reliably test that this approach
works?
> 
> Yup. With my BT headset. I'm backing out the change for now though to
> investigate other approaches separately. I'd rather get the rest of this in
> first to address crashes.

Acknowledged.

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/ios/objc/RTCAudioSession.h (right):

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/objc/RTCAudioSession.h:104: /** Setting this to
YES will cause the audio session to be configured for WebRTC
Perfect ;-)

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm (right):

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:242: OSStatus
result =
On 2016/05/05 23:23:28, tkchin_webrtc wrote:
> On 2016/05/04 12:33:07, henrika_webrtc wrote:
> > Perhaps a comment regarding why we need this listener.
> > 
> > Apple states "Listen for this property when you do care what the hardware
> sample
> > rate is and you want to match your audio processing to the hardware sample
> rate
> > (this is the typical case).
> > 
> > The Audio Unit will send you a notification when the hardware sample rate
> > changes. Your application will need to stop rendering, reconfigure any
> > processing being done for the new sample rate, then start rendering again.
The
> > sample rate change will be on the Output scope of Bus 0.
> > 
> > Your application should check the rate when called and only do any
> > reconfiguration when the new rate is different from the current rate. When
the
> > sample rates differ, stop rendering, reconfiguring your audio processing to
> the
> > new sample rate and then restart."
> 
> Removing these changes for now. Will address BT issues separately.

Acknowledged.

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:273: result =
On 2016/05/05 23:23:28, tkchin_webrtc wrote:
> On 2016/05/04 12:33:07, henrika_webrtc wrote:
> > Do we need a check that it is disabled at close?
> 
> Removing these changes for now. Will address BT issues separately.

Acknowledged.

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:341: void
VoiceProcessingAudioUnit::OnSampleRateChange(
On 2016/05/05 23:23:28, tkchin_webrtc wrote:
> On 2016/05/04 12:33:07, henrika_webrtc wrote:
> > Can we now get this callback and the routing change callback?
> > Conflicts?
> 
> There shouldn't be conflicts - we are always using the sample rate from the
> audio session.

Acknowledged.

https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:379: // We only
care about the output.
On 2016/05/05 23:23:28, tkchin_webrtc wrote:
> On 2016/05/04 12:33:07, henrika_webrtc wrote:
> > Have you really been able to trigger callback on other scope/bus?
> 
> Nope. But just in case.

Acknowledged.

[tkchin_webrtc, 2016-05-07 00:29:32]

On 2016/05/06 11:22:17, henrika_webrtc wrote:
> I think we should land this and take it from there. I can give it a test drive
> in AppRTCDemo then as well.
> 
> Zeke: what should I then focus on testing?
> 
> LGTM
> 
>
https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
> File webrtc/modules/audio_device/ios/audio_device_ios.mm (right):
> 
>
https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
> webrtc/modules/audio_device/ios/audio_device_ios.mm:534: // Mad hack:
Sometimes
> the audio session is already active because the app
> On 2016/05/05 23:23:27, tkchin_webrtc wrote:
> > On 2016/05/04 12:33:07, henrika_webrtc wrote:
> > > Wow. Do you have a case where you can reliably test that this approach
> works?
> > 
> > Yup. With my BT headset. I'm backing out the change for now though to
> > investigate other approaches separately. I'd rather get the rest of this in
> > first to address crashes.
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
> File webrtc/modules/audio_device/ios/objc/RTCAudioSession.h (right):
> 
>
https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
> webrtc/modules/audio_device/ios/objc/RTCAudioSession.h:104: /** Setting this
to
> YES will cause the audio session to be configured for WebRTC
> Perfect ;-)
> 
>
https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
> File webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm (right):
> 
>
https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
> webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:242: OSStatus
> result =
> On 2016/05/05 23:23:28, tkchin_webrtc wrote:
> > On 2016/05/04 12:33:07, henrika_webrtc wrote:
> > > Perhaps a comment regarding why we need this listener.
> > > 
> > > Apple states "Listen for this property when you do care what the hardware
> > sample
> > > rate is and you want to match your audio processing to the hardware sample
> > rate
> > > (this is the typical case).
> > > 
> > > The Audio Unit will send you a notification when the hardware sample rate
> > > changes. Your application will need to stop rendering, reconfigure any
> > > processing being done for the new sample rate, then start rendering again.
> The
> > > sample rate change will be on the Output scope of Bus 0.
> > > 
> > > Your application should check the rate when called and only do any
> > > reconfiguration when the new rate is different from the current rate. When
> the
> > > sample rates differ, stop rendering, reconfiguring your audio processing
to
> > the
> > > new sample rate and then restart."
> > 
> > Removing these changes for now. Will address BT issues separately.
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
> webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:273: result =
> On 2016/05/05 23:23:28, tkchin_webrtc wrote:
> > On 2016/05/04 12:33:07, henrika_webrtc wrote:
> > > Do we need a check that it is disabled at close?
> > 
> > Removing these changes for now. Will address BT issues separately.
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
> webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:341: void
> VoiceProcessingAudioUnit::OnSampleRateChange(
> On 2016/05/05 23:23:28, tkchin_webrtc wrote:
> > On 2016/05/04 12:33:07, henrika_webrtc wrote:
> > > Can we now get this callback and the routing change callback?
> > > Conflicts?
> > 
> > There shouldn't be conflicts - we are always using the sample rate from the
> > audio session.
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1945563003/diff/80001/webrtc/modules/audio_devi...
> webrtc/modules/audio_device/ios/voice_processing_audio_unit.mm:379: // We only
> care about the output.
> On 2016/05/05 23:23:28, tkchin_webrtc wrote:
> > On 2016/05/04 12:33:07, henrika_webrtc wrote:
> > > Have you really been able to trigger callback on other scope/bus?
> > 
> > Nope. But just in case.
> 
> Acknowledged.

Just that nothing is broken or crashing :) We should look at why BT isn't
working well in some cases though.

[tkchin_webrtc, 2016-05-07 00:51:34]

The CQ bit was checked by tkchin@webrtc.org

[tkchin_webrtc, 2016-05-07 00:51:35]

The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1945563003/#ps160001 (title:
"Remove unneeded volatile.")

[commit-bot: I haz the power, 2016-05-07 00:51:42]

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

[tkchin_webrtc, 2016-05-07 00:54:45]

Description was changed from

==========
Provide isAudioEnabled flag to control audio unit.

- Also removes async invoker usage in favor of thread posting
- Also responds to playback sample rate changes from audio unit

BUG=
==========

to

==========
Provide isAudioEnabled flag to control audio unit.

- Also removes async invoker usage in favor of thread posting

BUG=
==========

[commit-bot: I haz the power, 2016-05-07 01:54:18]

Description was changed from

==========
Provide isAudioEnabled flag to control audio unit.

- Also removes async invoker usage in favor of thread posting

BUG=
==========

to

==========
Provide isAudioEnabled flag to control audio unit.

- Also removes async invoker usage in favor of thread posting

BUG=
==========

[commit-bot: I haz the power, 2016-05-07 01:54:19]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-05-07 01:54:27]

Description was changed from

==========
Provide isAudioEnabled flag to control audio unit.

- Also removes async invoker usage in favor of thread posting

BUG=
==========

to

==========
Provide isAudioEnabled flag to control audio unit.

- Also removes async invoker usage in favor of thread posting

BUG=

Committed: https://crrev.com/d251196d377041722442fc583da593bc8f17302b
Cr-Commit-Position: refs/heads/master@{#12651}
==========

[commit-bot: I haz the power, 2016-05-07 01:54:29]

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

[henrika_webrtc, 2016-05-11 10:45:20]

FYI, Not sure if it related to this change but when I try AppRTCDemo today I
can't play the music file while in a call.
"Use manual audio config" does not make a difference. As I recall it, I was able
to run VoIP and music in a mixed mode earlier.
Or is that something I remembered wrong.