Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2996953002:
Make path to wav file for jitter buffer simulation in event_log_visualizer configurable. (Closed)
DescriptionMake path to wav file for jitter buffer simulation in event_log_visualizer configurable.
BUG=webrtc:7467
Review-Url: https://codereview.webrtc.org/2996953002
Cr-Commit-Position: refs/heads/master@{#19364}
Committed: https://chromium.googlesource.com/external/webrtc/+/7fbeb0b3c93f333c1a2277c2c5b1b3ccff6f9e77
Patch Set 1 #
Total comments: 6
Patch Set 2 : Adressed review comment and implemented 2nd option (use const std:string and assign ressource path … #
Total comments: 2
Patch Set 3 : Corrected bug introduced in previous patch set and reverted check for empty filename to first patch… #Patch Set 4 : Rebase #Messages
Total messages: 20 (11 generated)
Description was changed from ========== Make path to wav file for jitter buffer simulation in event_log_visualizer configurable. BUG= ========== to ========== Make path to wav file for jitter buffer simulation in event_log_visualizer configurable. BUG= ==========
owb@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Please take a look. Thank you.
Description was changed from ========== Make path to wav file for jitter buffer simulation in event_log_visualizer configurable. BUG= ========== to ========== Make path to wav file for jitter buffer simulation in event_log_visualizer configurable. BUG=webrtc:7467 ==========
https://codereview.webrtc.org/2996953002/diff/1/webrtc/rtc_tools/event_log_vi... File webrtc/rtc_tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2996953002/diff/1/webrtc/rtc_tools/event_log_vi... webrtc/rtc_tools/event_log_visualizer/main.cc:92: "", Is it possible to use webrtc::test::ResourcePath("audio_processing/conversational_speech/EN_script2_F_sp2_B1", "wav") as a default value here? If not, I have other suggestions below. https://codereview.webrtc.org/2996953002/diff/1/webrtc/rtc_tools/event_log_vi... webrtc/rtc_tools/event_log_visualizer/main.cc:247: std::string wav_path; If you can't set the flag default as above, I suggest you change to the following: const std::string wav_path = FLAG_wav_filename.empty() ? webrtc::test::ResourcePath("audio_processing/conversational_speech/EN_script2_F_sp2_B1", "wav") : FLAG_wav_filename;
https://codereview.webrtc.org/2996953002/diff/1/webrtc/rtc_tools/event_log_vi... File webrtc/rtc_tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2996953002/diff/1/webrtc/rtc_tools/event_log_vi... webrtc/rtc_tools/event_log_visualizer/main.cc:92: "", On 2017/08/15 11:54:45, hlundin-webrtc wrote: > Is it possible to use > webrtc::test::ResourcePath("audio_processing/conversational_speech/EN_script2_F_sp2_B1", > "wav") as a default value here? > > If not, I have other suggestions below. This does not seem to work and results in an undefined value for the flag. https://codereview.webrtc.org/2996953002/diff/1/webrtc/rtc_tools/event_log_vi... webrtc/rtc_tools/event_log_visualizer/main.cc:247: std::string wav_path; On 2017/08/15 11:54:45, hlundin-webrtc wrote: > If you can't set the flag default as above, I suggest you change to the > following: > > const std::string wav_path = FLAG_wav_filename.empty() ? > webrtc::test::ResourcePath("audio_processing/conversational_speech/EN_script2_F_sp2_B1", > "wav") : > FLAG_wav_filename; I implemented this version. FLAG_wav_filename is a character array, therefore I used strlen instead of .empty().
I wonder if you introduced a bug now... https://codereview.webrtc.org/2996953002/diff/1/webrtc/rtc_tools/event_log_vi... File webrtc/rtc_tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2996953002/diff/1/webrtc/rtc_tools/event_log_vi... webrtc/rtc_tools/event_log_visualizer/main.cc:92: "", On 2017/08/15 13:01:28, owb1 wrote: > On 2017/08/15 11:54:45, hlundin-webrtc wrote: > > Is it possible to use > > > webrtc::test::ResourcePath("audio_processing/conversational_speech/EN_script2_F_sp2_B1", > > "wav") as a default value here? > > > > If not, I have other suggestions below. > > This does not seem to work and results in an undefined value for the flag. Acknowledged. I thought so too, but figured that maybe some compiler magic could work it out. Now when I look at the implementation of ResourcePath, I see that it checks for the existence of files, which would be impossible to do accurately during compile time. https://codereview.webrtc.org/2996953002/diff/1/webrtc/rtc_tools/event_log_vi... webrtc/rtc_tools/event_log_visualizer/main.cc:247: std::string wav_path; On 2017/08/15 13:01:28, owb1 wrote: > On 2017/08/15 11:54:45, hlundin-webrtc wrote: > > If you can't set the flag default as above, I suggest you change to the > > following: > > > > const std::string wav_path = FLAG_wav_filename.empty() ? > > > webrtc::test::ResourcePath("audio_processing/conversational_speech/EN_script2_F_sp2_B1", > > "wav") : > > FLAG_wav_filename; > > I implemented this version. FLAG_wav_filename is a character array, therefore I > used strlen instead of .empty(). Oh, I thought it was a c++ string. I must be confusing it with gflags. Then your initial approach with an if-statement is not so bad. You can decide if you want to go back to that, or keep the current version with the conditional. https://codereview.webrtc.org/2996953002/diff/20001/webrtc/rtc_tools/event_lo... File webrtc/rtc_tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2996953002/diff/20001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/main.cc:253: analyzer.CreateAudioJitterBufferGraph(FLAG_wav_filename, 48000, This must be a bug. Now you are always using the FLAG value, even if it is empty, right?
LGTM with bug fixed.
owb@webrtc.org changed reviewers: + terelius@webrtc.org
Thanks for the comment. The bug should be fixed. https://codereview.webrtc.org/2996953002/diff/20001/webrtc/rtc_tools/event_lo... File webrtc/rtc_tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2996953002/diff/20001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/main.cc:253: analyzer.CreateAudioJitterBufferGraph(FLAG_wav_filename, 48000, On 2017/08/15 13:51:17, hlundin-webrtc wrote: > This must be a bug. Now you are always using the FLAG value, even if it is > empty, right? You are right. This was a bug. I reverted to the first patch set and left the check for an empty char array.
The CQ bit was checked by owb@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/2996953002/#ps40001 (title: "Corrected bug introduced in previous patch set and reverted check for empty filename to first patch…")
The CQ bit was unchecked by owb@webrtc.org
lgtm I think you need to rebase though.
The CQ bit was checked by owb@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2996953002/#ps60001 (title: "Rebase")
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": 60001, "attempt_start_ts": 1502874237880860,
"parent_rev": "2bf9e73e6bfcc854d5f9cfc6655f0595e53ae50a", "commit_rev":
"7fbeb0b3c93f333c1a2277c2c5b1b3ccff6f9e77"}
Message was sent while issue was closed.
Description was changed from ========== Make path to wav file for jitter buffer simulation in event_log_visualizer configurable. BUG=webrtc:7467 ========== to ========== Make path to wav file for jitter buffer simulation in event_log_visualizer configurable. BUG=webrtc:7467 Review-Url: https://codereview.webrtc.org/2996953002 Cr-Commit-Position: refs/heads/master@{#19364} Committed: https://chromium.googlesource.com/external/webrtc/+/7fbeb0b3c93f333c1a2277c2c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/7fbeb0b3c93f333c1a2277c2c... |
