Initial implementation of Android audio recording error handling.

Initial implementation of Android audio recording error handling.

BUG=b/34128648
R=henrika@webrtc.org

Review-Url: https://codereview.webrtc.org/2620453004 .
Cr-Commit-Position: refs/heads/master@{#16017}
Committed: https://chromium.googlesource.com/external/webrtc/+/1a9e2108e3197858b3f31c5380c3a3071c29f62e

[AlexG, 2017-01-06 21:12:31]

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

[henrika_webrtc, 2017-01-09 15:06:31]

Thanks Alex,

hope you are OK with my minor changes.

https://codereview.webrtc.org/2620453004/diff/1/webrtc/modules/audio_device/a...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java
(right):

https://codereview.webrtc.org/2620453004/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:65:
void onWebRtcAudioRecordStartError(String errorMessage);
I would rather use one for init and one for start. We do so today for our UMA
stats and it would be confusing if this interface added issues called "start"
when it was for "init".

Hence:

onWebRtcAudioRecordInitError
onWebRtcAudioRecordStartError

https://codereview.webrtc.org/2620453004/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:234:
reportWebRtcAudioRecordStartError("AudioRecord.startRecording failed - record
state");
Perhaps the notation "- record state" can be improved. FYI, this is the most
common state we will hit when another app is using the mic.

To separate error at line 230 with the error at line 234, why not keep 230 as is
and then use the exact issue on 234:

"audioRecord.getRecordingState() != AudioRecord.RECORDSTATE_RECORDING"

[henrika_webrtc, 2017-01-09 15:06:46]

Description was changed from

==========
Initial implementation of Android audio recording error handling.

BUG=b/34128648
==========

to

==========
Initial implementation of Android audio recording error handling.

BUG=b/34128648
==========

[AlexG, 2017-01-11 00:43:06]

https://codereview.webrtc.org/2620453004/diff/1/webrtc/modules/audio_device/a...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java
(right):

https://codereview.webrtc.org/2620453004/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:65:
void onWebRtcAudioRecordStartError(String errorMessage);
On 2017/01/09 15:06:31, henrika_webrtc wrote:
> I would rather use one for init and one for start. We do so today for our UMA
> stats and it would be confusing if this interface added issues called "start"
> when it was for "init".
> 
> Hence:
> 
> onWebRtcAudioRecordInitError
> onWebRtcAudioRecordStartError

Done.

https://codereview.webrtc.org/2620453004/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:234:
reportWebRtcAudioRecordStartError("AudioRecord.startRecording failed - record
state");
On 2017/01/09 15:06:31, henrika_webrtc wrote:
> Perhaps the notation "- record state" can be improved. FYI, this is the most
> common state we will hit when another app is using the mic.
> 
> To separate error at line 230 with the error at line 234, why not keep 230 as
is
> and then use the exact issue on 234:
> 
> "audioRecord.getRecordingState() != AudioRecord.RECORDSTATE_RECORDING"
> 

But we still need to report error in case of exception
catch (IllegalStateException e) {
}?

May be add separate event for this case or error code
void onWebRtcAudioRecordStartError(String errorMessage, int errorCode) ?

[henrika_webrtc, 2017-01-11 08:22:56]

LGTM

https://codereview.webrtc.org/2620453004/diff/1/webrtc/modules/audio_device/a...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java
(right):

https://codereview.webrtc.org/2620453004/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:65:
void onWebRtcAudioRecordStartError(String errorMessage);
On 2017/01/11 00:43:05, AlexG wrote:
> On 2017/01/09 15:06:31, henrika_webrtc wrote:
> > I would rather use one for init and one for start. We do so today for our
UMA
> > stats and it would be confusing if this interface added issues called
"start"
> > when it was for "init".
> > 
> > Hence:
> > 
> > onWebRtcAudioRecordInitError
> > onWebRtcAudioRecordStartError
> 
> Done.

Acknowledged.

https://codereview.webrtc.org/2620453004/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:234:
reportWebRtcAudioRecordStartError("AudioRecord.startRecording failed - record
state");
I have not seen that we ever hit the exception but let's keep it.
Using string only with unique explanation works for me.

https://codereview.webrtc.org/2620453004/diff/20001/webrtc/modules/audio_devi...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java
(right):

https://codereview.webrtc.org/2620453004/diff/20001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:173:
reportWebRtcAudioRecordInitError("InitRecording() called twice without
StopRecording()");
Nit, skip ending '()' to be consistent with the other error strings.

https://codereview.webrtc.org/2620453004/diff/20001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:318:
Logging.e(TAG, errorMessage);
Proposal: Logging.e(TAG, "Run-time recording error: " + errorMessage);

[AlexG, 2017-01-11 20:13:37]

https://codereview.webrtc.org/2620453004/diff/20001/webrtc/modules/audio_devi...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java
(right):

https://codereview.webrtc.org/2620453004/diff/20001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:173:
reportWebRtcAudioRecordInitError("InitRecording() called twice without
StopRecording()");
On 2017/01/11 08:22:56, henrika_webrtc wrote:
> Nit, skip ending '()' to be consistent with the other error strings.

Done.

https://codereview.webrtc.org/2620453004/diff/20001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:318:
Logging.e(TAG, errorMessage);
On 2017/01/11 08:22:56, henrika_webrtc wrote:
> Proposal: Logging.e(TAG, "Run-time recording error: " + errorMessage);

Done.

[AlexG, 2017-01-11 20:14:24]

The CQ bit was checked by glaznev@webrtc.org

[AlexG, 2017-01-11 20:14:24]

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/2620453004/#ps40001 (title:
"Address next comments")

[commit-bot: I haz the power, 2017-01-11 20:14:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[AlexG, 2017-01-11 21:00:04]

Description was changed from

==========
Initial implementation of Android audio recording error handling.

BUG=b/34128648
==========

to

==========
Initial implementation of Android audio recording error handling.

BUG=b/34128648
R=henrika@webrtc.org

Review-Url: https://codereview.webrtc.org/2620453004 .
Cr-Commit-Position: refs/heads/master@{#16017}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1a9e2108e3197858b3f31c538...
==========

[AlexG, 2017-01-11 21:00:05]

Committed patchset #3 (id:40001) manually as
1a9e2108e3197858b3f31c5380c3a3071c29f62e (presubmit successful).