Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2574053003:
Improves release of allocated audio resources on Android (Closed)
|
Created:
4 years ago by henrika_webrtc Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, AlexG Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImproves release of allocated audio resources on Android.
BUG=webrtc:6890
R=magjed@webrtc.org, solenberg@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/ac8d5164f0187d21da54905ed1143e6b04f5a7ab
Patch Set 1 #
Total comments: 5
Patch Set 2 : nit #Patch Set 3 : Feedback from solenberg@ #
Total comments: 6
Patch Set 4 : Removed thread-model change #
Total comments: 2
Patch Set 5 : restored one error check #
Messages
Total messages: 31 (15 generated)
Description was changed from ========== Improves release of allocated audio resources on Android BUG= ========== to ========== Improves release of allocated audio resources on Android BUG=webrtc:6890 ==========
henrika@webrtc.org changed reviewers: + solenberg@webrtc.org
The CQ bit was checked by henrika@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
henrika@webrtc.org changed reviewers: + glaznev@webrtc.org, magjed@webrtc.org
Description was changed from ========== Improves release of allocated audio resources on Android BUG=webrtc:6890 ========== to ========== Improves release of allocated audio resources on Android. This CL also moves call to AudioRecord.startRecording() to the audio thread to be consistent with the playout side. BUG=webrtc:6890 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.webrtc.org/2574053003/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/2574053003/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:84: Logging.e(TAG, "AudioRecord.startRecording failed: " + e.getMessage()); Should you call releaseAudioResources() here as well?
https://codereview.webrtc.org/2574053003/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioTrack.java (right): https://codereview.webrtc.org/2574053003/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioTrack.java:228: releaseAudioResources(); I'm not sure you should do this. The wrapping layer keeps a bool to indicate initialized state. I believe the protocol for the ADM is if (PlayoutIsInitialized()) then it's ok to call StartPlayout() So this particular change upsets that logic and the caller may legitimately think it's ok to StartPlayout() and fire the assertions at the beginning of this function.
Minor fix and an outstanding question. https://codereview.webrtc.org/2574053003/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/2574053003/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:84: Logging.e(TAG, "AudioRecord.startRecording failed: " + e.getMessage()); On 2016/12/14 16:29:04, magjed_webrtc wrote: > Should you call releaseAudioResources() here as well? Done. https://codereview.webrtc.org/2574053003/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioTrack.java (right): https://codereview.webrtc.org/2574053003/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioTrack.java:228: releaseAudioResources(); Will think more but that's how I interpreted your comment "Note that there’s another check in WebRtcAudioTrack::InitPlayout() which will throw us in the same state. It would be prudent to fix at the same time. If InitPlayout() fails, it should clean up any resources it has acquired. "
Off-line discussion works for me solenberg@
Off-line discussion works for me solenberg@
https://codereview.webrtc.org/2574053003/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioTrack.java (right): https://codereview.webrtc.org/2574053003/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioTrack.java:228: releaseAudioResources(); Wait. I get it now.
solenberg@: PTAL
henrika@webrtc.org changed reviewers: - glaznev@webrtc.org
https://codereview.webrtc.org/2574053003/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java (right): https://codereview.webrtc.org/2574053003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:84: Logging.e(TAG, "AudioRecord.startRecording failed: " + e.getMessage()); Does any logging of stats happen at the higher level - in whoever calls startRecording()? If that is the case we won't get that signal anymore. I'm wondering if you should do these changes in a separate CL? https://codereview.webrtc.org/2574053003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:220: Logging.e(TAG, "AudioRecord instance is not successfully initialized."); This could also mean it is in state RECORDSTATE_RECORDING etc. https://codereview.webrtc.org/2574053003/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioTrack.java (right): https://codereview.webrtc.org/2574053003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioTrack.java:227: if (audioTrack.getState() != AudioTrack.STATE_INITIALIZED) { Do you need this check? You already verify the state in initPlayout() and you verify here that audioThread is null. Also, like for record, the check isn't strictly correct - it could be PLAYSTATE_PAUSED etc and you'd still be logging a message that the track is not initialized...
Description was changed from ========== Improves release of allocated audio resources on Android. This CL also moves call to AudioRecord.startRecording() to the audio thread to be consistent with the playout side. BUG=webrtc:6890 ========== to ========== Improves release of allocated audio resources on Android. BUG=webrtc:6890 ==========
Great comments Fredrik. PTAL https://codereview.webrtc.org/2574053003/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java (right): https://codereview.webrtc.org/2574053003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:84: Logging.e(TAG, "AudioRecord.startRecording failed: " + e.getMessage()); Totally agree. Will do it in separate. I remember why there is a difference between play and rec here. Long and winding road. Removing. https://codereview.webrtc.org/2574053003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:220: Logging.e(TAG, "AudioRecord instance is not successfully initialized."); Removing https://codereview.webrtc.org/2574053003/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioTrack.java (right): https://codereview.webrtc.org/2574053003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioTrack.java:227: if (audioTrack.getState() != AudioTrack.STATE_INITIALIZED) { Agree. Can be confusing. Removing.
lgtm % comment https://codereview.webrtc.org/2574053003/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java (left): https://codereview.webrtc.org/2574053003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:216: if (audioRecord.getRecordingState() != AudioRecord.RECORDSTATE_RECORDING) { This check still serves a purpose though, doesn't it?
https://codereview.webrtc.org/2574053003/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java (left): https://codereview.webrtc.org/2574053003/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:216: if (audioRecord.getRecordingState() != AudioRecord.RECORDSTATE_RECORDING) { Good point. It is useful for e.g. mic conflict situations. Will restore before landing.
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2574053003/#ps80001 (title: "restored one error check")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/4646)
Message was sent while issue was closed.
Description was changed from ========== Improves release of allocated audio resources on Android. BUG=webrtc:6890 ========== to ========== Improves release of allocated audio resources on Android. BUG=webrtc:6890 R=magjed@webrtc.org, solenberg@webrtc.org Committed: https://crrev.com/ac8d5164f0187d21da54905ed1143e6b04f5a7ab Cr-Commit-Position: refs/heads/master@{#15637} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ac8d5164f0187d21da54905ed1143e6b04f5a7ab Cr-Commit-Position: refs/heads/master@{#15637}
Message was sent while issue was closed.
Description was changed from ========== Improves release of allocated audio resources on Android. BUG=webrtc:6890 R=magjed@webrtc.org, solenberg@webrtc.org Committed: https://crrev.com/ac8d5164f0187d21da54905ed1143e6b04f5a7ab Cr-Commit-Position: refs/heads/master@{#15637} ========== to ========== Improves release of allocated audio resources on Android. BUG=webrtc:6890 R=magjed@webrtc.org, solenberg@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/ac8d5164f0187d21da54905ed... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as ac8d5164f0187d21da54905ed1143e6b04f5a7ab (presubmit successful).
Message was sent while issue was closed.
Compile errors are unrelated to my CL. Landing manually. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
