Create iOS Audio Manager implementation.

Create iOS Audio Manager implementation.

The iOS audio manager supports audio input only (for voice search). This CL
also makes slight changes to audio_input_mac to make it usable on iOS.

BUG=None


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

[blundell, 2012-09-07 09:55:58]

Dale, please feel free to add any other reviewers as seems
appropriate/necessary. Thanks!

[stuartmorgan, 2012-09-07 11:09:26]

iOS and gyp parts LGTM with nits.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
File media/audio/ios/audio_manager_ios.h (right):

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.h:51: static bool isAudioSessionInitialized_;
This should be using c_style_variable_naming_, not objCStyle.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
File media/audio/ios/audio_manager_ios.mm (right):

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:15: static const int kMaxInputChannels = 2;
remove static, and put in an anonymous namespace.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:32: UInt32 audioInputIsAvailable = false;
All the local variables need style fixing too.

Also, this looks suspiciously like a bool, not a UInt32.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:40: return false;
Change to:

if (!InitAudioSession())
  return false;
<everything else>

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:61: }
Remove the {} for both

[DaleCurtis, 2012-09-07 11:31:12]

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
File media/audio/ios/audio_manager_ios.mm (right):

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:10: #include
"media/audio/ios/audio_manager_ios.h"
Should be first header; above everything.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:15: static const int kMaxInputChannels = 2;
On 2012/09/07 11:09:26, stuartmorgan wrote:
> remove static, and put in an anonymous namespace.

Better, just make it an enum { kMaxInputChannels = 2 }

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:89: void
AudioManagerIOS::ReleaseOutputStream(AudioOutputStream* stream) {
Should this have a NOTREACHED() ?

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:98: if (!isAudioSessionInitialized_) {
Do you expect to retry this call if it fails? Otherwise, just make this a
"static const bool" inside the function so you avoid any races; e.g.,

static bool InitializeAudioSessionInternal() {
...
}

bool AudioManagerIOS::InitAudioSession() {
  static const bool kSessionInitialized = InitializeAudioSessionInternal();
  return kSessionInitialized;
}

This is safe because function level static initialization is guaranteed atomic.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/mac/aud...
File media/audio/mac/audio_input_mac.h (right):

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/mac/aud...
media/audio/mac/audio_input_mac.h:17: class AudioManagerBase;
Why change these?

[stuartmorgan, 2012-09-07 11:35:29]

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/mac/aud...
File media/audio/mac/audio_input_mac.h (right):

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/mac/aud...
media/audio/mac/audio_input_mac.h:17: class AudioManagerBase;
On 2012/09/07 11:31:12, DaleCurtis wrote:
> Why change these?

iOS uses AudioManagerIOS; otherwise the implementation is completely shareable
between Mac and iOS. Since nothing in this class requires anything but the AMB
interface, this lets us share the class between Mac and iOS without ifdef-ing
the manager type.

[blundell, 2012-09-07 12:35:39]

PTAL, thanks!

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
File media/audio/ios/audio_manager_ios.h (right):

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.h:51: static bool isAudioSessionInitialized_;
On 2012/09/07 11:09:26, stuartmorgan wrote:
> This should be using c_style_variable_naming_, not objCStyle.

Done.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
File media/audio/ios/audio_manager_ios.mm (right):

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:10: #include
"media/audio/ios/audio_manager_ios.h"
On 2012/09/07 11:31:12, DaleCurtis wrote:
> Should be first header; above everything.

Done.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:15: static const int kMaxInputChannels = 2;
On 2012/09/07 11:09:26, stuartmorgan wrote:
> remove static, and put in an anonymous namespace.

Done.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:15: static const int kMaxInputChannels = 2;
On 2012/09/07 11:31:12, DaleCurtis wrote:
> On 2012/09/07 11:09:26, stuartmorgan wrote:
> > remove static, and put in an anonymous namespace.
> 
> Better, just make it an enum { kMaxInputChannels = 2 }

Done.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:32: UInt32 audioInputIsAvailable = false;
On 2012/09/07 11:09:26, stuartmorgan wrote:
> All the local variables need style fixing too.
> 
> Also, this looks suspiciously like a bool, not a UInt32.

Done. This property is actually a 32-bit integer that is interpreted as a
boolean; changing audio_input_is_available to be a boolean causes the result to
not be set correctly. Added some explanation and error-checking code.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:40: return false;
On 2012/09/07 11:09:26, stuartmorgan wrote:
> Change to:
> 
> if (!InitAudioSession())
>   return false;
> <everything else>

Done.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:61: }
On 2012/09/07 11:09:26, stuartmorgan wrote:
> Remove the {} for both

Done.

https://chromiumcodereview.appspot.com/10907110/diff/2001/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:98: if (!isAudioSessionInitialized_) {
Much cleaner, thanks. Done.

On 2012/09/07 11:31:12, DaleCurtis wrote:
> Do you expect to retry this call if it fails? Otherwise, just make this a
> "static const bool" inside the function so you avoid any races; e.g.,
> 
> static bool InitializeAudioSessionInternal() {
> ...
> }
> 
> bool AudioManagerIOS::InitAudioSession() {
>   static const bool kSessionInitialized = InitializeAudioSessionInternal();
>   return kSessionInitialized;
> }
> 
> This is safe because function level static initialization is guaranteed
atomic.

[stuartmorgan, 2012-09-07 12:47:34]

https://chromiumcodereview.appspot.com/10907110/diff/2002/media/audio/ios/aud...
File media/audio/ios/audio_manager_ios.mm (right):

https://chromiumcodereview.appspot.com/10907110/diff/2002/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:35: // 32-bit integer, not a boolean
(http://developer.apple.com/library/ios/#documentation/AudioToolbox/Reference/...).
Remove this link; Apple's docs are notorious for changing the URL without
redirecting.

[blundell, 2012-09-07 12:53:06]

https://chromiumcodereview.appspot.com/10907110/diff/2002/media/audio/ios/aud...
File media/audio/ios/audio_manager_ios.mm (right):

https://chromiumcodereview.appspot.com/10907110/diff/2002/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:35: // 32-bit integer, not a boolean
(http://developer.apple.com/library/ios/#documentation/AudioToolbox/Reference/...).
On 2012/09/07 12:47:34, stuartmorgan wrote:
> Remove this link; Apple's docs are notorious for changing the URL without
> redirecting.

Done.

[DaleCurtis, 2012-09-07 12:55:58]

LGTM % nits that can be ignored if you'd prefer the current style.

https://chromiumcodereview.appspot.com/10907110/diff/2002/media/audio/ios/aud...
File media/audio/ios/audio_manager_ios.mm (right):

https://chromiumcodereview.appspot.com/10907110/diff/2002/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:16: enum {
nit: We generally place this all on one line for a single value.

https://chromiumcodereview.appspot.com/10907110/diff/2002/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:105: static bool InitAudioSessionInternal()
{
nit: static functions at the top of the file.

[blundell, 2012-09-07 13:03:21]

Thanks.

https://chromiumcodereview.appspot.com/10907110/diff/2002/media/audio/ios/aud...
File media/audio/ios/audio_manager_ios.mm (right):

https://chromiumcodereview.appspot.com/10907110/diff/2002/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:16: enum {
On 2012/09/07 12:55:58, DaleCurtis wrote:
> nit: We generally place this all on one line for a single value.

Done.

https://chromiumcodereview.appspot.com/10907110/diff/2002/media/audio/ios/aud...
media/audio/ios/audio_manager_ios.mm:105: static bool InitAudioSessionInternal()
{
On 2012/09/07 12:55:58, DaleCurtis wrote:
> nit: static functions at the top of the file.

Done.

[blundell, 2012-09-07 13:03:53]

+tommi for OWNERS.

[tommi (sloooow) - chröme, 2012-09-07 13:22:35]

lgtm

[commit-bot: I haz the power, 2012-09-07 13:24:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/10907110/8

[commit-bot: I haz the power, 2012-09-07 15:09:27]

Try job failure for 10907110-8 (retry) on mac_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-07 15:14:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/10907110/8

[commit-bot: I haz the power, 2012-09-07 17:39:00]

Change committed as 155416