Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2486763002:
Add support to audioproc_f for running the residual echo detector and producing an echo likelihood … (Closed)
|
Created:
4 years, 1 month ago by ivoc Modified:
4 years, 1 month ago 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 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd support to audioproc_f for running the residual echo detector and producing an echo likelihood graph.
This adds two command-line flags to audioproc_f: -red and -red_graph, which can be used to enable/disable the RED, and to set the output path for the graph. The graph is generated as a python script that depends on matplotlib and numpy to display the graph.
BUG=webrtc:6525
Committed: https://crrev.com/87d1a787547b26fd564b66426116c2252aa8233d
Cr-Commit-Position: refs/heads/master@{#15069}
Patch Set 1 : Initial version #
Total comments: 10
Patch Set 2 : Review comments by Per. #
Total comments: 8
Patch Set 3 : Review comments by Henrik. #
Total comments: 2
Patch Set 4 : Added RTC_CHECK to ensure file has opened #
Total comments: 2
Patch Set 5 : Added missing include. #
Messages
Total messages: 22 (8 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add support to audioproc_f for running the residual echo detector and producing an echo likelihood graph. This adds two command-line flags to audioproc_f: -red and -red_graph, which can be used to enable/disable the RED, and to set the output path for the graph. The graph is generated as a python script that depends on matplotlib and numpy to display the graph. BUG=webrtc:6525 ========== to ========== Add support to audioproc_f for running the residual echo detector and producing an echo likelihood graph. This adds two command-line flags to audioproc_f: -red and -red_graph, which can be used to enable/disable the RED, and to set the output path for the graph. The graph is generated as a python script that depends on matplotlib and numpy to display the graph. BUG=webrtc:6525 ==========
ivoc@webrtc.org changed reviewers: + peah@webrtc.org
Hi Per, please have a look.
ivoc@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:192: intelligibility_enhancer_enabled_ || Great catch! I missed that when reviewing the other CL. This is not really part of this CL though so I think you should either 1) Change the description of this CL to reflect that this is done as well. 2) Make another CL that does this. I would lean towards 2) as the rest of the CL relates to simulation code and this relates to production code. Furthermore, the correction below needs to be added as well. https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:209: return RenderMultiBandProcessingActive() || echo_canceller_enabled_ || You need to add "|| residual_echo_detector_enabled_" here as well as the render signal copy will otherwise not be generated. https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:68: residual_echo_likelihood_graph_ << "import numpy as np" << std::endl Would it be possible to abstract this printout into a local function named something like WriteEchoLikelihoodGraphFileHeader. I think that would simplify the code, and implicitly add code documentation (via the function name) https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:77: residual_echo_likelihood_graph_ << "])" << std::endl Would it be possible to abstract this printout into a local function named something like WriteEchoLikelihoodGraphFileFooter. I think that would simplify the code, and implicitly add code documentation (via the function name) https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:174: std::ofstream residual_echo_likelihood_graph_; What about changing the name to residual_echo_likelihood_graph_writer_? The name residual_echo_likelihood_graph_ is a bit misleading as it is referring to a file (stream) and not a graph.
Thanks for the comments! https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:192: intelligibility_enhancer_enabled_ || On 2016/11/10 10:47:09, peah-webrtc wrote: > Great catch! I missed that when reviewing the other CL. > > This is not really part of this CL though so I think you should either > 1) Change the description of this CL to reflect that this is done as well. > 2) Make another CL that does this. > > I would lean towards 2) as the rest of the CL relates to simulation code and > this relates to production code. > > Furthermore, the correction below needs to be added as well. Good point, I will make a new CL for these changes. https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:209: return RenderMultiBandProcessingActive() || echo_canceller_enabled_ || On 2016/11/10 10:47:09, peah-webrtc wrote: > You need to add "|| residual_echo_detector_enabled_" here as well as the render > signal copy will otherwise not be generated. Good point. I will add it to the other CL. https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:68: residual_echo_likelihood_graph_ << "import numpy as np" << std::endl On 2016/11/10 10:47:10, peah-webrtc wrote: > Would it be possible to abstract this printout into a local function named > something like > WriteEchoLikelihoodGraphFileHeader. > > I think that would simplify the code, and implicitly add code documentation (via > the function name) Ok, done. https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:77: residual_echo_likelihood_graph_ << "])" << std::endl On 2016/11/10 10:47:09, peah-webrtc wrote: > Would it be possible to abstract this printout into a local function named > something like > WriteEchoLikelihoodGraphFileFooter. > > I think that would simplify the code, and implicitly add code documentation (via > the function name) Done. https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:174: std::ofstream residual_echo_likelihood_graph_; On 2016/11/10 10:47:10, peah-webrtc wrote: > What about changing the name to residual_echo_likelihood_graph_writer_? > The name residual_echo_likelihood_graph_ is a bit misleading as it is referring > to a file (stream) and not a graph. Good idea, done.
On 2016/11/10 15:36:08, ivoc wrote: > Thanks for the comments! > > https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:192: > intelligibility_enhancer_enabled_ || > On 2016/11/10 10:47:09, peah-webrtc wrote: > > Great catch! I missed that when reviewing the other CL. > > > > This is not really part of this CL though so I think you should either > > 1) Change the description of this CL to reflect that this is done as well. > > 2) Make another CL that does this. > > > > I would lean towards 2) as the rest of the CL relates to simulation code and > > this relates to production code. > > > > Furthermore, the correction below needs to be added as well. > > Good point, I will make a new CL for these changes. > > https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:209: return > RenderMultiBandProcessingActive() || echo_canceller_enabled_ || > On 2016/11/10 10:47:09, peah-webrtc wrote: > > You need to add "|| residual_echo_detector_enabled_" here as well as the > render > > signal copy will otherwise not be generated. > > Good point. I will add it to the other CL. > > https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): > > https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/test/audio_processing_simulator.cc:68: > residual_echo_likelihood_graph_ << "import numpy as np" << std::endl > On 2016/11/10 10:47:10, peah-webrtc wrote: > > Would it be possible to abstract this printout into a local function named > > something like > > WriteEchoLikelihoodGraphFileHeader. > > > > I think that would simplify the code, and implicitly add code documentation > (via > > the function name) > > Ok, done. > > https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/test/audio_processing_simulator.cc:77: > residual_echo_likelihood_graph_ << "])" << std::endl > On 2016/11/10 10:47:09, peah-webrtc wrote: > > Would it be possible to abstract this printout into a local function named > > something like > > WriteEchoLikelihoodGraphFileFooter. > > > > I think that would simplify the code, and implicitly add code documentation > (via > > the function name) > > Done. > > https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): > > https://codereview.webrtc.org/2486763002/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/test/audio_processing_simulator.h:174: > std::ofstream residual_echo_likelihood_graph_; > On 2016/11/10 10:47:10, peah-webrtc wrote: > > What about changing the name to residual_echo_likelihood_graph_writer_? > > The name residual_echo_likelihood_graph_ is a bit misleading as it is > referring > > to a file (stream) and not a graph. > > Good idea, done. Great! lgtm
LGTM with some questions and nits. https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:45: void WriteEchoLikelihoodGraphFileHeader(std::ofstream& output_file) { Non-const reference violates style guide. https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:83: residual_echo_likelihood_graph_writer_.open( Can open() fail? https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:49: rtc::Optional<bool> use_red; Can you spell out "[residual] echo detector" in an inline comment, just to reduce the risk of confusing it with the RED payload format? https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:75: "Activate (1) or deactivate(0) the residual echo detector"); Space between 'deactivate' and '('. (This is a copy-pasted error.)
Thanks! https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:45: void WriteEchoLikelihoodGraphFileHeader(std::ofstream& output_file) { On 2016/11/14 10:38:28, hlundin-webrtc wrote: > Non-const reference violates style guide. Changed this into a pointer. https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:83: residual_echo_likelihood_graph_writer_.open( On 2016/11/14 10:38:28, hlundin-webrtc wrote: > Can open() fail? Good point, I added a check for this to avoid writing the header in that case. https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:49: rtc::Optional<bool> use_red; On 2016/11/14 10:38:28, hlundin-webrtc wrote: > Can you spell out "[residual] echo detector" in an inline comment, just to > reduce the risk of confusing it with the RED payload format? Done. https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/2486763002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:75: "Activate (1) or deactivate(0) the residual echo detector"); On 2016/11/14 10:38:28, hlundin-webrtc wrote: > Space between 'deactivate' and '('. > (This is a copy-pasted error.) Done.
https://codereview.webrtc.org/2486763002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2486763002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:85: if (residual_echo_likelihood_graph_writer_.is_open()) { Since this is test code, I would argue that you should crash and burn if this fails, rather than leaving the user puzzled as to why s/he gets no file output.
https://codereview.webrtc.org/2486763002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2486763002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:85: if (residual_echo_likelihood_graph_writer_.is_open()) { On 2016/11/14 12:57:35, hlundin-webrtc wrote: > Since this is test code, I would argue that you should crash and burn if this > fails, rather than leaving the user puzzled as to why s/he gets no file output. Good point, I changed this into an RTC_CHECK().
https://codereview.webrtc.org/2486763002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2486763002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:85: RTC_CHECK(residual_echo_likelihood_graph_writer_.is_open()); Great! Now all you have to do is remember to #include "webrtc/base/checks.h". :)
https://codereview.webrtc.org/2486763002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2486763002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:85: RTC_CHECK(residual_echo_likelihood_graph_writer_.is_open()); On 2016/11/14 14:23:11, hlundin-webrtc wrote: > Great! Now all you have to do is remember to #include "webrtc/base/checks.h". :) Done. I saw all those other CHECKS in the file so I didn't bother to check the includes.
Still lgtm.
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/2486763002/#ps100001 (title: "Added missing include.")
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 ========== Add support to audioproc_f for running the residual echo detector and producing an echo likelihood graph. This adds two command-line flags to audioproc_f: -red and -red_graph, which can be used to enable/disable the RED, and to set the output path for the graph. The graph is generated as a python script that depends on matplotlib and numpy to display the graph. BUG=webrtc:6525 ========== to ========== Add support to audioproc_f for running the residual echo detector and producing an echo likelihood graph. This adds two command-line flags to audioproc_f: -red and -red_graph, which can be used to enable/disable the RED, and to set the output path for the graph. The graph is generated as a python script that depends on matplotlib and numpy to display the graph. BUG=webrtc:6525 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add support to audioproc_f for running the residual echo detector and producing an echo likelihood graph. This adds two command-line flags to audioproc_f: -red and -red_graph, which can be used to enable/disable the RED, and to set the output path for the graph. The graph is generated as a python script that depends on matplotlib and numpy to display the graph. BUG=webrtc:6525 ========== to ========== Add support to audioproc_f for running the residual echo detector and producing an echo likelihood graph. This adds two command-line flags to audioproc_f: -red and -red_graph, which can be used to enable/disable the RED, and to set the output path for the graph. The graph is generated as a python script that depends on matplotlib and numpy to display the graph. BUG=webrtc:6525 Committed: https://crrev.com/87d1a787547b26fd564b66426116c2252aa8233d Cr-Commit-Position: refs/heads/master@{#15069} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/87d1a787547b26fd564b66426116c2252aa8233d Cr-Commit-Position: refs/heads/master@{#15069} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
