Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2182043005:
Adds periodic volume-level logging for Android (Closed)
|
Created:
4 years, 4 months ago by henrika_webrtc Modified:
4 years, 4 months ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdds periodic volume-level logging for Android.
The goal of this change is to log the volume level for the
current audio stream so we can keep track of what volume the
user selects during a call.
BUG=b/30376577
R=magjed@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/c62ff860230aa416b780114aa0cd14dedf7dc3b5
Patch Set 1 #Patch Set 2 : nit #
Total comments: 14
Patch Set 3 : Feedback from magjed@ #
Total comments: 1
Messages
Total messages: 17 (9 generated)
Description was changed from ========== Adds periodic volume-level logging for Android BUG= ========== to ========== Adds periodic volume-level logging for Android BUG=b/30376577 ==========
henrika@webrtc.org changed reviewers: + magjed@webrtc.org
Description was changed from ========== Adds periodic volume-level logging for Android BUG=b/30376577 ========== to ========== Adds periodic volume-level logging for Android. The goal of this change is to log the volume level for the current audio stream so we can keep track of what volum the user selects during a call. BUG=b/30376577 ==========
Description was changed from ========== Adds periodic volume-level logging for Android. The goal of this change is to log the volume level for the current audio stream so we can keep track of what volum the user selects during a call. BUG=b/30376577 ========== to ========== Adds periodic volume-level logging for Android. The goal of this change is to log the volume level for the current audio stream so we can keep track of what volume the user selects during a call. BUG=b/30376577 ==========
PTAL
It would be better with a volume change listener and only log when the volume changes, but if that is impossible or too complicated, this is fine. lgtm if you address my comments. https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java (right): https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:90: private VolumeLogger volumeLogger; make this variable final https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:96: private class VolumeLogger { make this class static (if possible) https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:96: private class VolumeLogger { move this class to the other static variables, e.g. after AUDIO_MODES https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:100: private AudioManager audioManager; make this variable final https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:115: class LogVolumeTask extends TimerTask { make this class private static https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:136: + AUDIO_MODES[audioManager.getMode()]); replace 'audioManager.getMode()' with just mode here. https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:142: timer.cancel(); I think it's safest to do: if (timer != null) { timer.cancel(); timer = null; } I see that you only call stop() from dispose() if it's been initialized, so this null check is strictly not necessary in the current code, but it might save you from a silly null pointer exception in the future.
Thanks! https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java (right): https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:90: private VolumeLogger volumeLogger; On 2016/07/28 09:44:22, magjed_webrtc wrote: > make this variable final Done. https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:96: private class VolumeLogger { On 2016/07/28 09:44:22, magjed_webrtc wrote: > move this class to the other static variables, e.g. after AUDIO_MODES Done. https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:96: private class VolumeLogger { On 2016/07/28 09:44:22, magjed_webrtc wrote: > make this class static (if possible) Done. https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:100: private AudioManager audioManager; On 2016/07/28 09:44:22, magjed_webrtc wrote: > make this variable final Done. https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:115: class LogVolumeTask extends TimerTask { private but not static works ;-) https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:136: + AUDIO_MODES[audioManager.getMode()]); On 2016/07/28 09:44:22, magjed_webrtc wrote: > replace 'audioManager.getMode()' with just mode here. Done. https://codereview.webrtc.org/2182043005/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:142: timer.cancel(); On 2016/07/28 09:44:22, magjed_webrtc wrote: > I think it's safest to do: > if (timer != null) { > timer.cancel(); > timer = null; > } > I see that you only call stop() from dispose() if it's been initialized, so this > null check is strictly not necessary in the current code, but it might save you > from a silly null pointer exception in the future. Done.
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 Link to the patchset: https://codereview.webrtc.org/2182043005/#ps40001 (title: "Feedback from magjed@")
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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
Description was changed from ========== Adds periodic volume-level logging for Android. The goal of this change is to log the volume level for the current audio stream so we can keep track of what volume the user selects during a call. BUG=b/30376577 ========== to ========== Adds periodic volume-level logging for Android. The goal of this change is to log the volume level for the current audio stream so we can keep track of what volume the user selects during a call. BUG=b/30376577 R=magjed@webrtc.org Committed: https://crrev.com/c62ff860230aa416b780114aa0cd14dedf7dc3b5 Cr-Commit-Position: refs/heads/master@{#13555} ==========
Message was sent while issue was closed.
Description was changed from ========== Adds periodic volume-level logging for Android. The goal of this change is to log the volume level for the current audio stream so we can keep track of what volume the user selects during a call. BUG=b/30376577 ========== to ========== Adds periodic volume-level logging for Android. The goal of this change is to log the volume level for the current audio stream so we can keep track of what volume the user selects during a call. BUG=b/30376577 R=magjed@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/c62ff860230aa416b780114aa... ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c62ff860230aa416b780114aa0cd14dedf7dc3b5 Cr-Commit-Position: refs/heads/master@{#13555}
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as c62ff860230aa416b780114aa0cd14dedf7dc3b5 (presubmit successful).
Message was sent while issue was closed.
https://codereview.webrtc.org/2182043005/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java (right): https://codereview.webrtc.org/2182043005/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:88: timer = new Timer(THREAD_NAME); On second thought, this actually creates a new native thread and that's quite expensive. |
