Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2883283002:
Add log message to help analyze why echo likelihood > 1.1 (Closed)
|
Created:
3 years, 7 months ago by ivoc Modified:
3 years, 3 months ago Reviewers:
peah-webrtc CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd log message to help analyze why echo likelihood > 1.1
This CL adds a log message with the relevant part of the internal state of the echo detector to the text log when this unexpected scenario occurs.
BUG=b/38014838
Review-Url: https://codereview.webrtc.org/2883283002
Cr-Commit-Position: refs/heads/master@{#18185}
Committed: https://chromium.googlesource.com/external/webrtc/+/1592c74d8a5613baa0a0853abeca7f9d81712dbe
Patch Set 1 #
Total comments: 10
Patch Set 2 : Review comments. #Patch Set 3 : Added variable to header file #Patch Set 4 : Added static_cast #Patch Set 5 : Fixed rebasing issues #
Messages
Total messages: 19 (10 generated)
ivoc@webrtc.org changed reviewers: + peah@webrtc.org
Hi Per, PTAL at this CL to add a message to the text log when the echo likelihood is unexpectedly high.
Description was changed from ========== Add log message to debug echo likelihood values > 1.1 This CL adds a log message with the relevant part of the internal state of the echo detector to the text log when this unexpected scenario occurs. BUG=b/38014838 ========== to ========== Add log message to help analyze why echo likelihood > 1.1 This CL adds a log message with the relevant part of the internal state of the echo detector to the text log when this unexpected scenario occurs. BUG=b/38014838 ==========
Awesome! lgtm, conditional that you have a look at the comments. https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/residual_echo_detector.cc:129: best_delay = delay; I guess that the assignment in line 129 may fail on some platforms, as the result may overflow (right?) You probably should do a static cast, or use an optional<size_t> instead (but that is probably more expensive). https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/residual_echo_detector.cc:135: if (echo_likelihood_ > 1.1f) { Not fully sure about the static variable. I'd rather have chosen to have a member variable instead, but I cannot see anything in the style guide that forbids that. https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/residual_echo_detector.cc:140: (kLookbackFrames + next_insertion_index_ - best_delay) % I just want to point out that the % operator involves a division. Please consider using a conditional instead. https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/residual_echo_detector.cc:143: LOG_F(LS_ERROR) << "Unexpected high echo likelihood value: " Please consider using j-son type formatting, as in level_controller.cc. That will simplify machine parsing of the log. That is a feedback that I've gotten which I think makes sense, but in this case is probably not super necessary as the logging is temporary. However, as you are listing a lot of variable values, json-style may make sense in particular for that.
Thanks for the comments! https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/residual_echo_detector.cc:129: best_delay = delay; On 2017/05/17 04:26:15, peah-webrtc wrote: > I guess that the assignment in line 129 may fail on some platforms, as the > result may overflow (right?) You probably should do a static cast, or use an > optional<size_t> instead (but that is probably more expensive). I don't think it's possible for this assignment to fail on any platform. The "delay" variable runs from 0 to kLookbackFrames, which is 650 (covariances_ has a fixed size), which can easily be represented as an int. Since this is only a temporary change and this is a simple solution, I would prefer to leave it like this if that's okay with you. https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/residual_echo_detector.cc:135: if (echo_likelihood_ > 1.1f) { On 2017/05/17 04:26:15, peah-webrtc wrote: > Not fully sure about the static variable. I'd rather have chosen to have a > member variable instead, but I cannot see anything in the style guide that > forbids that. The reason I chose a static variable is so that all of the code for this log message is localized here. But you're right that a member variable is a nicer solution, so I changed this to be a member variable instead. https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/residual_echo_detector.cc:140: (kLookbackFrames + next_insertion_index_ - best_delay) % On 2017/05/17 04:26:15, peah-webrtc wrote: > I just want to point out that the % operator involves a division. Please > consider using a conditional instead. You're right that divisions are slow, but this piece of code can be executed at most 5 times, so it is not very performance critical. I also think that actually logging the message will be much slower than a single division. I did change this to an if statement instead of the division. https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/residual_echo_detector.cc:143: LOG_F(LS_ERROR) << "Unexpected high echo likelihood value: " On 2017/05/17 04:26:15, peah-webrtc wrote: > Please consider using j-son type formatting, as in level_controller.cc. That > will simplify machine parsing of the log. That is a feedback that I've gotten > which I think makes sense, but in this case is probably not super necessary as > the logging is temporary. However, as you are listing a lot of variable values, > json-style may make sense in particular for that. That seems like a good idea, done.
https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/residual_echo_detector.cc:129: best_delay = delay; On 2017/05/17 09:28:07, ivoc wrote: > On 2017/05/17 04:26:15, peah-webrtc wrote: > > I guess that the assignment in line 129 may fail on some platforms, as the > > result may overflow (right?) You probably should do a static cast, or use an > > optional<size_t> instead (but that is probably more expensive). > > I don't think it's possible for this assignment to fail on any platform. The > "delay" variable runs from 0 to kLookbackFrames, which is 650 (covariances_ has > a fixed size), which can easily be represented as an int. Since this is only a > temporary change and this is a simple solution, I would prefer to leave it like > this if that's okay with you. Sorry, I meant that the compiler will complain. As you mention, the assignment will not actually overflow. That is why I suggested the static_cast.
https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2883283002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/residual_echo_detector.cc:129: best_delay = delay; On 2017/05/17 09:38:07, peah-webrtc wrote: > On 2017/05/17 09:28:07, ivoc wrote: > > On 2017/05/17 04:26:15, peah-webrtc wrote: > > > I guess that the assignment in line 129 may fail on some platforms, as the > > > result may overflow (right?) You probably should do a static cast, or use an > > > optional<size_t> instead (but that is probably more expensive). > > > > I don't think it's possible for this assignment to fail on any platform. The > > "delay" variable runs from 0 to kLookbackFrames, which is 650 (covariances_ > has > > a fixed size), which can easily be represented as an int. Since this is only a > > temporary change and this is a simple solution, I would prefer to leave it > like > > this if that's okay with you. > > Sorry, I meant that the compiler will complain. As you mention, the assignment > will not actually overflow. That is why I suggested the static_cast. Ah, that's a good point. Added static_cast.
The CQ bit was checked by ivoc@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/2883283002/#ps60001 (title: "Added static_cast")
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: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/21544)
The CQ bit was checked by ivoc@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/2883283002/#ps80001 (title: "Fixed rebasing issues")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495038571062340,
"parent_rev": "76a5593835af6a6696125973005c487b4d7cf76b", "commit_rev":
"1592c74d8a5613baa0a0853abeca7f9d81712dbe"}
Message was sent while issue was closed.
Description was changed from ========== Add log message to help analyze why echo likelihood > 1.1 This CL adds a log message with the relevant part of the internal state of the echo detector to the text log when this unexpected scenario occurs. BUG=b/38014838 ========== to ========== Add log message to help analyze why echo likelihood > 1.1 This CL adds a log message with the relevant part of the internal state of the echo detector to the text log when this unexpected scenario occurs. BUG=b/38014838 Review-Url: https://codereview.webrtc.org/2883283002 Cr-Commit-Position: refs/heads/master@{#18185} Committed: https://chromium.googlesource.com/external/webrtc/+/1592c74d8a5613baa0a0853ab... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/1592c74d8a5613baa0a0853ab...
Message was sent while issue was closed.
Patchset #6 (id:100001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
