Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104273002:
Add logging to Intelligibility Enhancer (Closed)
|
Created:
4 years, 5 months ago by aluebs-webrtc Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd logging to Intelligibility Enhancer
It logs when the IE is activated and deactivated.
R=ivoc@webrtc.org, turaj@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/1aa821980d2f871336d2f323143934bb81affff6
Patch Set 1 #Patch Set 2 : Log active percentage #
Total comments: 3
Patch Set 3 : Log num_chunks_ when activating and deactivating #
Total comments: 2
Patch Set 4 : Fix log #Patch Set 5 : Rebasing #
Messages
Total messages: 22 (10 generated)
aluebs@webrtc.org changed reviewers: + turaj@webrtc.org
LGTM. I had a thought when you were talking about logging. To make the post processing of logs easier, does it make sense that IE keeps track of the time it has been engaged and log a single number at dtor representing the active percentage. Then one has to pull only that single number for each call.
On 2016/06/29 16:24:21, turaj wrote: > LGTM. > > I had a thought when you were talking about logging. To make the post processing > of logs easier, does it make sense that IE keeps track of the time it has been > engaged and log a single number at dtor representing the active percentage. Then > one has to pull only that single number for each call. Great suggestion! What do you think of this implementation?
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, ivoc@webrtc.org
Adding ivoc@ to weigh in on logging in the dtor (last patch set). https://codereview.webrtc.org/2104273002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/2104273002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:116: LOG(LS_INFO) << "Intelligibility Enhancer was active for " The dtor is not always executed at the end of a call. At least in Chrome, a fast shutdown is performed if the user just closes the tab. +Ivo will know more about this. But in essence, beware that you might not get the logging you expect.
Description was changed from ========== Add logging to Intelligibility Enhancer It logs when the IE is activated and deactivated. ========== to ========== Add logging to Intelligibility Enhancer It logs when the IE is activated and deactivated. ==========
henrik.lundin@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
https://codereview.webrtc.org/2104273002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/2104273002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:116: LOG(LS_INFO) << "Intelligibility Enhancer was active for " On 2016/06/30 08:59:36, hlundin-webrtc wrote: > The dtor is not always executed at the end of a call. At least in Chrome, a fast > shutdown is performed if the user just closes the tab. +Ivo will know more about > this. But in essence, beware that you might not get the logging you expect. Indeed, don't rely on destructors being called in Chrome. It may make more sense to log this periodically instead.
Patchset #3 (id:40001) has been deleted
https://codereview.webrtc.org/2104273002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/2104273002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:116: LOG(LS_INFO) << "Intelligibility Enhancer was active for " On 2016/06/30 09:13:48, ivoc wrote: > On 2016/06/30 08:59:36, hlundin-webrtc wrote: > > The dtor is not always executed at the end of a call. At least in Chrome, a > fast > > shutdown is performed if the user just closes the tab. +Ivo will know more > about > > this. But in essence, beware that you might not get the logging you expect. > > Indeed, don't rely on destructors being called in Chrome. It may make more sense > to log this periodically instead. Thank you for pointing this out. I have been testing this a bit and it seems it runs the destructor if the user hangs up, but it doesn't if they directly kill the app from the app-switcher. I will leave this here, since I think it adds value, but also added the num_chunk_ when the IE is activated and deactivated so this percentage can be calculated from there if the destructor was not run.
LGTM, see my comment below. https://codereview.webrtc.org/2104273002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/2104273002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:199: LOG(LS_INFO) << "Intelligibility Enhancer was activated at chunk " This should probably say "deactivated".
The CQ bit was checked by aluebs@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from turaj@webrtc.org, ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2104273002/#ps80001 (title: "Fix log")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2104273002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/2104273002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:199: LOG(LS_INFO) << "Intelligibility Enhancer was activated at chunk " On 2016/07/01 08:04:30, ivoc wrote: > This should probably say "deactivated". Good catch. Done.
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 ========== Add logging to Intelligibility Enhancer It logs when the IE is activated and deactivated. ========== to ========== Add logging to Intelligibility Enhancer It logs when the IE is activated and deactivated. R=ivoc@webrtc.org, turaj@webrtc.org Committed: https://crrev.com/1aa821980d2f871336d2f323143934bb81affff6 Cr-Commit-Position: refs/heads/master@{#13370} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1aa821980d2f871336d2f323143934bb81affff6 Cr-Commit-Position: refs/heads/master@{#13370}
Message was sent while issue was closed.
Description was changed from ========== Add logging to Intelligibility Enhancer It logs when the IE is activated and deactivated. R=ivoc@webrtc.org, turaj@webrtc.org Committed: https://crrev.com/1aa821980d2f871336d2f323143934bb81affff6 Cr-Commit-Position: refs/heads/master@{#13370} ========== to ========== Add logging to Intelligibility Enhancer It logs when the IE is activated and deactivated. R=ivoc@webrtc.org, turaj@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/1aa821980d2f871336d2f3231... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) manually as 1aa821980d2f871336d2f323143934bb81affff6 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
