Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1991723002:
AEC: Add UMA logging of buffer re-alignment (Closed)
|
Created:
4 years, 7 months ago by hlundin-webrtc Modified:
4 years, 7 months ago Reviewers:
peah-webrtc CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAEC: Add UMA logging of buffer re-alignment
This change adds a UMA log that will be written to when a non-zero delay
correction is done in the AEC. The number of elements moved (positive or
negative) will be logged to
"WebRTC.Audio.AecDelayAdjustmentAgnosticValue" or
"WebRTC.Audio.AecDelayAdjustmentSystemValue", depending on whether
delay-agnostic AEC is used or not, respectively.
BUG=webrtc:5903
Committed: https://crrev.com/83e8286c6bbf5a30f8e4a4b373ef708566f9da07
Cr-Commit-Position: refs/heads/master@{#12795}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Updated #
Total comments: 2
Messages
Total messages: 12 (4 generated)
henrik.lundin@webrtc.org changed reviewers: + peah@webrtc.org
Per, please, take a look. https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1816: MaybeLogDelayAdjustment(moved_elements, DelaySource::kSystemDelay); What is the unit for moved_elements? Should probably convert to ms.
On 2016/05/18 08:04:30, hlundin-webrtc wrote: > Per, please, take a look. > > https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/aec/aec_core.cc (right): > > https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/aec/aec_core.cc:1816: > MaybeLogDelayAdjustment(moved_elements, DelaySource::kSystemDelay); > What is the unit for moved_elements? Should probably convert to ms. lgtm with the nit that you should also look whether it would make sense to log something in echo_cancellatio.cc since you are in this CL also adding logging for non-delay-agnostic-related delay changes.
https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:64: } Is a default case needed, or is it fine to use like this when we are switching agains an enum? https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1816: MaybeLogDelayAdjustment(moved_elements, DelaySource::kSystemDelay); On 2016/05/18 08:04:30, hlundin-webrtc wrote: > What is the unit for moved_elements? Should probably convert to ms. For moved_elements below, each element corresponds to 4 ms in sample rate >= 16 kHz, and 8 ms in 8 kHz. For this line, I'm not sure, but it would make sense to assume it is the same. Please doublecheck my statement above! https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1822: MaybeLogDelayAdjustment(moved_elements, DelaySource::kDelayAgnostic); There is also delay adjustments being applied in echo_cancellation.cc (in the EstBufDelayExtended(...) method). It could possibly make sense to log something there as well?
https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:64: } On 2016/05/18 08:22:11, peah-webrtc wrote: > Is a default case needed, or is it fine to use like this when we are switching > agains an enum? I believe it is fine. The compiler complains when I do not handle any of the enum values. https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1816: MaybeLogDelayAdjustment(moved_elements, DelaySource::kSystemDelay); On 2016/05/18 08:22:11, peah-webrtc wrote: > On 2016/05/18 08:04:30, hlundin-webrtc wrote: > > What is the unit for moved_elements? Should probably convert to ms. > > For moved_elements below, each element corresponds to 4 ms in sample rate >= 16 > kHz, and 8 ms in 8 kHz. > > For this line, I'm not sure, but it would make sense to assume it is the same. > > Please doublecheck my statement above! Done. https://codereview.webrtc.org/1991723002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1822: MaybeLogDelayAdjustment(moved_elements, DelaySource::kDelayAgnostic); On 2016/05/18 08:22:11, peah-webrtc wrote: > There is also delay adjustments being applied in echo_cancellation.cc (in the > EstBufDelayExtended(...) method). It could possibly make sense to log something > there as well? Acknowledged. We'll keep that as a separate exercise.
https://codereview.webrtc.org/1991723002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1991723002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:51: if (moved_ms == 0) I'd rather put this check when calling the method. It is more explicit I think, and cheaper. But I'm fully fine with how it is here as well. https://codereview.webrtc.org/1991723002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1816: MaybeLogDelayAdjustment(moved_elements * (aec->sampFreq == 8000 ? 8 : 4), I'd rather put the if-statement contained inside the method around the emthod call, i.e., if (moved_elements > 0) { MaybeLogDelayAdjustment(); } but I leave it up to you to decide that.
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/1991723002/#ps20001 (title: "Updated")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1991723002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1991723002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== AEC: Add UMA logging of buffer re-alignment This change adds a UMA log that will be written to when a non-zero delay correction is done in the AEC. The number of elements moved (positive or negative) will be logged to "WebRTC.Audio.AecDelayAdjustmentAgnosticValue" or "WebRTC.Audio.AecDelayAdjustmentSystemValue", depending on whether delay-agnostic AEC is used or not, respectively. BUG=webrtc:5903 ========== to ========== AEC: Add UMA logging of buffer re-alignment This change adds a UMA log that will be written to when a non-zero delay correction is done in the AEC. The number of elements moved (positive or negative) will be logged to "WebRTC.Audio.AecDelayAdjustmentAgnosticValue" or "WebRTC.Audio.AecDelayAdjustmentSystemValue", depending on whether delay-agnostic AEC is used or not, respectively. BUG=webrtc:5903 Committed: https://crrev.com/83e8286c6bbf5a30f8e4a4b373ef708566f9da07 Cr-Commit-Position: refs/heads/master@{#12795} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/83e8286c6bbf5a30f8e4a4b373ef708566f9da07 Cr-Commit-Position: refs/heads/master@{#12795} |
