Cleanup of iOS AudioDevice implementation

Cleanup of iOS AudioDevice implementation

TBR=tkchin
BUG=webrtc:4789
TEST=modules_unittests --gtest_filter=AudioDeviceTest* and AppRTCDemo

Committed: https://chromium.googlesource.com/external/webrtc/+/ba35d05a4918b3efa7ab88674781aadb48017ff8

[henrika_webrtc, 2015-06-24 08:24:55]

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

[henrika_webrtc, 2015-06-24 08:26:39]

Uploaded a new CL here instead. Please use this one. Thanks.

[henrika_webrtc, 2015-06-25 16:12:32]

Improved how we derive the fundamental audio parameters. A new method is called
at construction but I override the results directly to ensure that I don't
do any functional changes in this CL. The new method still adds lots of good
logging.

Also, fixed a real issue which I found when I added better logging. Today, we
actually fail to set the mode to AVAudioSessionModeVoiceChat since the
category is default. If we first set the category and then set the mode, things
are OK. Not sure, but it might mean that we don't do EC as we should today.
Anyhow, fixed in this CL.

[tkchin_webrtc, 2015-07-06 03:46:29]

https://codereview.webrtc.org/1206783002/diff/40001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/audio_device_generic.h (right):

https://codereview.webrtc.org/1206783002/diff/40001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/audio_device_generic.h:150: // Windows Core Audio
and Android only.
nit: if adding periods, add them to all comments in the file.

https://codereview.webrtc.org/1206783002/diff/40001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/ios/audio_device_ios.h (right):

https://codereview.webrtc.org/1206783002/diff/40001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/ios/audio_device_ios.h:2: *  Copyright (c) 2015 The
WebRTC project authors. All Rights Reserved.
I believe the rule is to never change the copyright year on existing files.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/audio_device_generic.cc (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/audio_device_generic.cc:18:
WEBRTC_TRACE(kTraceError, kTraceAudioDevice, -1,
These traces were removed in the other file, did you want to remove them here
too (or convert them to base/logging.h)?

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/audio_device_impl.cc (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/audio_device_impl.cc:328: ptrAudioDevice = new
AudioDeviceIOS();
nit: indentation

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

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:2: *  Copyright (c) 2015 The
WebRTC project authors. All Rights Reserved.
I don't think that copyright years are supposed to be changed on files.

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

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:25: #define TAG
"AudioDeviceIOS::"
Instead of using TAG why not introduce a logging function/macro that will look
at __FILE__, __PRETTY_FUNCTION__, __LINE__ etc?

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:40:
CHECK(CheckAndLogError(success, errorCode));
Seems a bit much to crash the app if there is an error. Also it's not quite an
error code that is being returned, but an error object with code + description.

There's not much you can do in error so logging it + returning is sufficient.
Maybe DCHECK.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:50:
CHECK(CheckAndLogError(success, errorCode));
ditto DCHECK

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:59: CHECK([[session
category]
style nit: access properties using dot syntax
session.category, session.mode

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:81: double sample_rate =
(double)[session sampleRate];
ditto dot syntax for properties

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h:2: * 
Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
ditto don't change copyright years

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h:23: class
AudioDeviceNotImplementedIOS : public AudioDeviceGeneric {
Instead of introducing an intermediate abstract class, it may be cleaner to just
leave the unimplemented methods in their own section in the AudioDeviceIOS
header file. You can still keep the unimplemented methods in a separate .cc
file.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.mm
(right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.mm:30: FATAL()
<< "Not implemented";
RTC_NOTREACHED?

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
File webrtc/modules/utility/source/helpers_ios.mm (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:41: NSLog(@"Error: %ld, %@",
(long)error.code, error.localizedDescription);
I think it is better to stick with webrtc LOG macros for logging (Hoping to
convert the rest of our bits from NSLog -> LOG). They will show up in console in
debug builds. And you can set a flag if you want them enabled in Release builds
with the rest of the webrtc logs.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:48: NSString* name = [NSString
stringWithFormat:@"%@", [NSThread currentThread]];
I would add these functions to logging.cc so that it logs thread info when you
set LogThreads(true)

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:65: AVAudioSession*
GetAudioSession() {
Is this necessary since it's a one-liner?

[henrika_webrtc, 2015-07-07 16:01:39]

Comments and question only. No new upload at this stage (did not have time to do
all changes, marked some as "Will fix").
Would be great with some more feedback. Then I can upload a new version
tomorrow.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/audio_device_generic.cc (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/audio_device_generic.cc:18:
WEBRTC_TRACE(kTraceError, kTraceAudioDevice, -1,
Let me fix it ;-)

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/audio_device_impl.cc (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/audio_device_impl.cc:328: ptrAudioDevice = new
AudioDeviceIOS();
On 2015/07/06 03:46:28, tkchin_webrtc wrote:
> nit: indentation

Done.

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

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.h:2: *  Copyright (c) 2015 The
WebRTC project authors. All Rights Reserved.
Fine, thanks ;-)

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

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:25: #define TAG
"AudioDeviceIOS::"
Will do. Thanks.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:40:
CHECK(CheckAndLogError(success, errorCode));
Will fix.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:50:
CHECK(CheckAndLogError(success, errorCode));
Will fix.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:59: CHECK([[session
category]
Will fix.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:81: double sample_rate =
(double)[session sampleRate];
Will fix.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h:2: * 
Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
Please note that this file is new. Not sure why we see a diff against an
unrelated file here. Keeping 2015 since the file is in fact created now.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h:23: class
AudioDeviceNotImplementedIOS : public AudioDeviceGeneric {
Forgive me for being lazy here but I was advised to use this type of solution
for a very similar case in Chrome. Then I started out as you propose but
changed. Do you feel strongly about changing the existing way?

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.mm
(right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.mm:30: FATAL()
<< "Not implemented";
On 2015/07/06 03:46:29, tkchin_webrtc wrote:
> RTC_NOTREACHED?

Done.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
File webrtc/modules/utility/source/helpers_ios.mm (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:41: NSLog(@"Error: %ld, %@",
(long)error.code, error.localizedDescription);
On 2015/07/06 03:46:29, tkchin_webrtc wrote:
> I think it is better to stick with webrtc LOG macros for logging (Hoping to
> convert the rest of our bits from NSLog -> LOG). They will show up in console
in
> debug builds. And you can set a flag if you want them enabled in Release
builds
> with the rest of the webrtc logs.

Done.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:48: NSString* name = [NSString
stringWithFormat:@"%@", [NSThread currentThread]];
Great idea, let me add support for GetThreadId as well then. But hope it is OK
if I keep GetCurrentThreadDescription() here since it is a special format that I
only log once at construction. Example: <NSThread: 0x170261100>{number = 1, name
= main}
A default thread ID looks like: [tid=2055] only.

Also, do you know where to call rtc::LogMessage::LogThreads() or how to add
command-line flag to add thread IDs to the logs? I tried unittest_main.cc but it
had no effect AFAIK.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:65: AVAudioSession*
GetAudioSession() {
Removed.

[henrika_webrtc, 2015-07-08 15:20:10]

Great comments; thanks.

New version uploaded. PTAL ;-)

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

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:40:
CHECK(CheckAndLogError(success, errorCode));
On 2015/07/07 16:01:38, henrika_webrtc wrote:
> Will fix.

Done.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:50:
CHECK(CheckAndLogError(success, errorCode));
On 2015/07/07 16:01:38, henrika_webrtc wrote:
> Will fix.

Done.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:59: CHECK([[session
category]
On 2015/07/07 16:01:38, henrika_webrtc wrote:
> Will fix.

Done.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:81: double sample_rate =
(double)[session sampleRate];
On 2015/07/07 16:01:38, henrika_webrtc wrote:
> Will fix.

Done.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
File webrtc/modules/utility/source/helpers_ios.mm (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:41: NSLog(@"Error: %ld, %@",
(long)error.code, error.localizedDescription);
Not sure if my way is what you wanted. Hope it looks OK.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:48: NSString* name = [NSString
stringWithFormat:@"%@", [NSThread currentThread]];
Found a way. Done. Given new calls in the added unit test, we now log lines
like:

[000:001] [2055] AudioDeviceIOS::Init

since I have added both extra timestamp and thread id here. Can be removed in
the unit test if needed.

[tkchin_webrtc, 2015-07-08 19:41:14]

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

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:25: #define TAG
"AudioDeviceIOS::"
On 2015/07/07 16:01:38, henrika_webrtc wrote:
> Will do. Thanks.

I looked at logging.cc and for some reason the LOG macro takes the filename+line
number but doesn't actually do anything with it. How strange. I'd rather have it
be fixed there rather then here, but this works for me for now.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h:2: * 
Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
On 2015/07/07 16:01:38, henrika_webrtc wrote:
> Please note that this file is new. Not sure why we see a diff against an
> unrelated file here. Keeping 2015 since the file is in fact created now.

whoops, my bad.

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h:23: class
AudioDeviceNotImplementedIOS : public AudioDeviceGeneric {
On 2015/07/07 16:01:38, henrika_webrtc wrote:
> Forgive me for being lazy here but I was advised to use this type of solution
> for a very similar case in Chrome. Then I started out as you propose but
> changed. Do you feel strongly about changing the existing way?

I think intermediate class makes sense if there are multiple subclasses, but
here we're only ever going to have one since it is iOS specific. Would say that
removing a class makes AudioDeviceIOS more readable and easier to maintain :)
Sorry!

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
File webrtc/modules/utility/source/helpers_ios.mm (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:41: NSLog(@"Error: %ld, %@",
(long)error.code, error.localizedDescription);
On 2015/07/08 15:20:10, henrika_webrtc wrote:
> Not sure if my way is what you wanted. Hope it looks OK.

Yeah, just wanted LS_ERROR. Format is up to you, looks fine to me.

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

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:34: NSError* errorObject =
nil;
nit: usually just
NSError* error = nil;

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:48: error:&errorObject];
nit: I think it is good practice to explicitly set error = nil before reusing,
here and below.

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/utility/i...
File webrtc/modules/utility/interface/helpers_ios.h (right):

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/utility/i...
webrtc/modules/utility/interface/helpers_ios.h:31: std::string
GetCurrentThreadDescription();
I think you can get the name using pthread methods, might be nice to put that in
platform_thread.h.
Fine to keep this here too, but maybe add TODO alluding to the above.

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/utility/s...
File webrtc/modules/utility/source/helpers_ios.mm (right):

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:13: #import
<AVFoundation/AVFoundation.h>
Don't think you need AVFoundation here anymore.

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:15: #include <pthread.h>
ditto no pthread bits here

[henrika_webrtc, 2015-07-09 12:58:02]

I think we are in phase now ;-)

Thanks!

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h (right):

https://codereview.webrtc.org/1206783002/diff/100001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_not_implemented_ios.h:23: class
AudioDeviceNotImplementedIOS : public AudioDeviceGeneric {
Nop. Will remove this class ;-)

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

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:34: NSError* errorObject =
nil;
On 2015/07/08 19:41:14, tkchin_webrtc wrote:
> nit: usually just
> NSError* error = nil;

Done.

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/ios/audio_device_ios.mm:48: error:&errorObject];
In fact, I tried CheckAndLogError(BOOL success, NSError** error) to be able to
set *error to nil inside and then call with CheckAndLogError(success, &error),
but for some reason linking failed then. The goal was to avoid adding several
lines of "error = nil;" lines.

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/utility/i...
File webrtc/modules/utility/interface/helpers_ios.h (right):

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/utility/i...
webrtc/modules/utility/interface/helpers_ios.h:31: std::string
GetCurrentThreadDescription();
On 2015/07/08 19:41:14, tkchin_webrtc wrote:
> I think you can get the name using pthread methods, might be nice to put that
in
> platform_thread.h.
> Fine to keep this here too, but maybe add TODO alluding to the above.

Acknowledged.

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/utility/s...
File webrtc/modules/utility/source/helpers_ios.mm (right):

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:13: #import
<AVFoundation/AVFoundation.h>
On 2015/07/08 19:41:14, tkchin_webrtc wrote:
> Don't think you need AVFoundation here anymore.

Done.

https://codereview.webrtc.org/1206783002/diff/120001/webrtc/modules/utility/s...
webrtc/modules/utility/source/helpers_ios.mm:15: #include <pthread.h>
On 2015/07/08 19:41:14, tkchin_webrtc wrote:
> ditto no pthread bits here

Done.

[henrika_webrtc, 2015-07-10 13:29:50]

Also added some extra device logging which I think will be useful. Example
output:

[2055] LogDeviceInfo
[2055]  system name: iPhone OS
[2055]  system version: 8.4
[2055]  device type: iPhone
[2055]  device name: iPhone 6

And a friendly ping for final review ;-)

[henrika_webrtc, 2015-07-14 15:03:48]

Landing with tkchin@ as TBR. Hope that's fine. Do ping me if there were some
final things you want changed.

Thanks ;-)

[henrika_webrtc, 2015-07-14 15:04:26]

Committed patchset #13 (id:240001) manually as
ba35d05a4918b3efa7ab88674781aadb48017ff8 (presubmit successful).

[henrika_webrtc, 2015-07-14 15:05:11]

Sorry, forgot that we had CQ.

[tkchin_webrtc, 2015-07-14 18:16:29]

On 2015/07/14 15:05:11, henrika_webrtc wrote:
> Sorry, forgot that we had CQ.

lgtm