Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2278393002:
Added logging of the level controller metrics (Closed)
|
Created:
4 years, 3 months ago by peah-webrtc Modified:
4 years, 3 months ago Reviewers:
hlundin-webrtc CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded logging of the level controller metrics.
BUG=
Committed: https://crrev.com/3026ee81d381b010336c3ea7c30b0fc920ca3a56
Cr-Commit-Position: refs/heads/master@{#13939}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Changes in response to reviewer comments #Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Rebase #
Messages
Total messages: 30 (15 generated)
Description was changed from ========== # Enter a description of the change. Added logging of the level controller metrics BUG= ========== to ========== Added logging of the level controller metrics. BUG= ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:111: int max_noise_power_dbfs = static_cast<int>( I would argue that you should make all the local variables you calculate here const int, just to make it clear that it is the same value that is logged to both RTC_HISTOGRAMS and LOG. https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:112: 10 * log10(max_noise_energy_ / frame_length_ + 1e-10f) - kdBFSOffset); You are dividing by frame_length_ in a few places. I guess it can never be zero, but please consider documenting that with an RTC_DCHECK. https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:169: << "Last term peak level: " Last -> Long https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:233: float frame_peak_level = PeakLevel(*audio); const https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:234: float long_term_peak_level = const https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/level_controller.h (right): https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.h:46: void Update(float peak_level, This is renamed to long_term_peak_level in the .cc file. Please, unify.
https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:111: int max_noise_power_dbfs = static_cast<int>( On 2016/08/26 07:53:28, hlundin-webrtc wrote: > I would argue that you should make all the local variables you calculate here > const int, just to make it clear that it is the same value that is logged to > both RTC_HISTOGRAMS and LOG. Done. https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:112: 10 * log10(max_noise_energy_ / frame_length_ + 1e-10f) - kdBFSOffset); On 2016/08/26 07:53:28, hlundin-webrtc wrote: > You are dividing by frame_length_ in a few places. I guess it can never be zero, > but please consider documenting that with an RTC_DCHECK. Done. https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:169: << "Last term peak level: " On 2016/08/26 07:53:29, hlundin-webrtc wrote: > Last -> Long Done. https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:233: float frame_peak_level = PeakLevel(*audio); On 2016/08/26 07:53:29, hlundin-webrtc wrote: > const Done. https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:234: float long_term_peak_level = On 2016/08/26 07:53:29, hlundin-webrtc wrote: > const Done. https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/level_controller.h (right): https://codereview.webrtc.org/2278393002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.h:46: void Update(float peak_level, On 2016/08/26 07:53:29, hlundin-webrtc wrote: > This is renamed to long_term_peak_level in the .cc file. Please, unify. Done.
Nice. LGTM.
The CQ bit was checked by peah@webrtc.org
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: ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/2224)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2278393002/#ps40001 (title: "Rebase")
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_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/1...)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2278393002/#ps80001 (title: "Rebase")
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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/15876)
The CQ bit was checked by peah@webrtc.org
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)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Added logging of the level controller metrics. BUG= ========== to ========== Added logging of the level controller metrics. BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Added logging of the level controller metrics. BUG= ========== to ========== Added logging of the level controller metrics. BUG= Committed: https://crrev.com/3026ee81d381b010336c3ea7c30b0fc920ca3a56 Cr-Commit-Position: refs/heads/master@{#13939} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3026ee81d381b010336c3ea7c30b0fc920ca3a56 Cr-Commit-Position: refs/heads/master@{#13939} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
