Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790933002:
Conversational speech tool, simualtor + unit tests (Closed)
|
Created:
3 years, 8 months ago by AleBzk Modified:
3 years, 6 months ago Reviewers:
minyue-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. |
DescriptionThe simulator puts into action the schedule of speech turns encoded in a MultiEndCall instance. The output is a set of audio track pairs. There is one set for each speaker and each set contains one near-end and one far-end audio track. The tracks are directly written into wav files instead of creating them in memory. To speed up the creation of the output wav files, *all* the source audio tracks (i.e., the atomic speech turns) are pre-loaded.
The ConversationalSpeechTest.MultiEndCallSimulator unit test defines a conversational speech sequence and creates two wav files (with pure tones at 440 and 880 Hz) that are used as atomic speech turn tracks.
This CL also patches MultiEndCall in order to allow input audio tracks with same sample rate and single channel only.
BUG=webrtc:7218
Review-Url: https://codereview.webrtc.org/2790933002
Cr-Commit-Position: refs/heads/master@{#18480}
Committed: https://chromium.googlesource.com/external/webrtc/+/6b648c4697cede14605fd2b89425866eec5f7c79
Patch Set 1 #Patch Set 2 : Using AppendFolder() to build paths to folders #
Total comments: 34
Patch Set 3 : merge AND comments from Minyue addressed #Patch Set 4 : map iterators simplified #
Total comments: 40
Patch Set 5 : comments from Minyue addressed #Patch Set 6 : SpeakerOutputFilePaths ctor fixed #Patch Set 7 : var name fixed #Patch Set 8 : fix after merge #Patch Set 9 : win trybot failure isolated #Patch Set 10 : debugging DeleteFolderAndContents #Patch Set 11 : file del win error #Patch Set 12 : UT workaround due to webrtc:7769 bug #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 30 (12 generated)
Description was changed from ========== Conversational speech tool, simualtor + unit tests Same sample rate and mono audio tracks only. BUG=webrtc:7218 ========== to ========== The simulator puts into action the schedule of speech turns encoded in a MultiEndCall instance. The output is a set of audio track pairs. There is one set for each speaker and each set contains one near-end and one far-end audio track. The tracks are directly written into wav files instead of creating them in memory. To speed up the creation of the output wav files, *all* the source audio tracks (i.e., the atomic speech turns) are pre-loaded. The ConversationalSpeechTest.MultiEndCallSimulator unit test defines a conversational speech sequence and creates two wav files (with pure tones at 440 and 880 Hz) that are used as atomic speech turn tracks. This CL also patches MultiEndCall in order to allow input audio tracks with same sample rate and single channel only. BUG=webrtc:7218 ==========
alessiob@webrtc.org changed reviewers: + minyue@webrtc.org
PTAL
I may missed some details. But take a look at my comments so far. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:137: for (auto it = sine_tracks_params.begin(); it != sine_tracks_params.end(); for (const auto& sine_tracks_param : sine_tracks_params) https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:663: for (auto it = generated_audiotrak_pairs->begin(); same here https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.cc (right): https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:35: std::unique_ptr<std::map<std::string, SpeakerOutputFilePaths>> I don't know the benefit of passing a unique_ptr of a map. Map has clear copy-ctor, passing by value should not be bad. WDYT? https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:81: // RTC_DISALLOW_COPY_AND_ASSIGN(SpeakerWavWriters); why commented out? https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:130: // previously written samples in wav_writer is less than interval.begin, it adds |interval_begin|, (1. underscore, 2. put || on var names) https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:133: void PadLeftWriteChunk(const std::vector<int16_t>& source_samples, consider using ArrayView instead of vector https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:134: std::size_t interval_begin, WavWriter* wav_writer) { no need for std:: https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:138: RTC_CHECK(padding_size >= 0); size_t never is negative RTC_CHECK_GE(interval_begin, wav_writer->num_samples()) before subtraction https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:139: if (padding_size > 0) { no need for "if" if you check before https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:151: void PadRight(WavWriter* wav_writer, std::size_t duration_samples) { duration is normally a time concept. Say either duration_ms or simply pad_samples https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:151: void PadRight(WavWriter* wav_writer, std::size_t duration_samples) { This looks similar to PadLeftWriteChunk but function names differ too much https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:154: RTC_CHECK(padding_size >= 0); again, checking something always true :) https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:165: std::unique_ptr<std::map<std::string, SpeakerOutputFilePaths>> Simulate( Are you not gonna write this in any class? https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:165: std::unique_ptr<std::map<std::string, SpeakerOutputFilePaths>> Simulate( and same comment on unique_ptr<map> https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.h (right): https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:29: : near_end(std::move(new_near_end)), no need for "move" https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:32: std::string near_end; can add const https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:33: std::string far_end; can add const https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:35: // RTC_DISALLOW_COPY_AND_ASSIGN(SpeakerOutputFilePaths); why commented out
Thanks Minyue. My apologies for the additional changes due to a merge that I have done before answering you by mistake. I answered to all your comments. You only commented simulator.cc/.h and generator_unittest.cc. Let's discuss offline the std::unique_ptr point you raised. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:137: for (auto it = sine_tracks_params.begin(); it != sine_tracks_params.end(); On 2017/04/07 13:24:30, minyue-webrtc wrote: > for (const auto& sine_tracks_param : sine_tracks_params) Nope. I need both key and value from the map. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:663: for (auto it = generated_audiotrak_pairs->begin(); On 2017/04/07 13:24:30, minyue-webrtc wrote: > same here Same here as well :) https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.cc (right): https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:35: std::unique_ptr<std::map<std::string, SpeakerOutputFilePaths>> On 2017/04/07 13:24:30, minyue-webrtc wrote: > I don't know the benefit of passing a unique_ptr of a map. Map has clear > copy-ctor, passing by value should not be bad. WDYT? I'm not a move semantics expert, but I'd say that in this case returning a unique pointer should be more lightweight than copying a map - the map keys are strings, the values are pairs of strings. However, it's also true that the map is not expected huge. But I don't see too much overhead in the code for using unique pointers. This is why I personally prefer this solution. But please, let me know if the comparison above (namely, passing unique ptr vs copying a map) is wrong. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:81: // RTC_DISALLOW_COPY_AND_ASSIGN(SpeakerWavWriters); On 2017/04/07 13:24:30, minyue-webrtc wrote: > why commented out? yet another leftover. Removed. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:130: // previously written samples in wav_writer is less than interval.begin, it adds On 2017/04/07 13:24:31, minyue-webrtc wrote: > |interval_begin|, (1. underscore, 2. put || on var names) Done. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:133: void PadLeftWriteChunk(const std::vector<int16_t>& source_samples, On 2017/04/07 13:24:30, minyue-webrtc wrote: > consider using ArrayView instead of vector Done. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:134: std::size_t interval_begin, WavWriter* wav_writer) { On 2017/04/07 13:24:30, minyue-webrtc wrote: > no need for std:: Done. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:138: RTC_CHECK(padding_size >= 0); On 2017/04/07 13:24:30, minyue-webrtc wrote: > size_t never is negative > > RTC_CHECK_GE(interval_begin, wav_writer->num_samples()) before subtraction Done. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:139: if (padding_size > 0) { On 2017/04/07 13:24:30, minyue-webrtc wrote: > no need for "if" if you check before I still need it because padding_size can be 0. If so, I don't have to write anything. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:151: void PadRight(WavWriter* wav_writer, std::size_t duration_samples) { On 2017/04/07 13:24:30, minyue-webrtc wrote: > duration is normally a time concept. Say either duration_ms or simply > pad_samples duration_samples -> pad_samples done PadRight -> PadRightWrite done I'm not using Chunk here because only zeros are written and no chunk is copied - as done instead in PadLeftWriteChunk. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:154: RTC_CHECK(padding_size >= 0); On 2017/04/07 13:24:31, minyue-webrtc wrote: > again, checking something always true :) Done. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:165: std::unique_ptr<std::map<std::string, SpeakerOutputFilePaths>> Simulate( On 2017/04/07 13:24:31, minyue-webrtc wrote: > and same comment on unique_ptr<map> Are you not gonna write this in any class? No. In this way, I'm hiding a lot of includes in the .cc file making .h lightweight to include. unique_ptr<map> See my previous answer. It applies here as well. Let me know what you think. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.h (right): https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:29: : near_end(std::move(new_near_end)), On 2017/04/07 13:24:31, minyue-webrtc wrote: > no need for "move" Well, last week Henrik, Karl and I came across this point. See here: https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... We ended up to using std::move to have one copy and one move instead of two copies. This is apparently a good solution when using emplace operations from the standard library - which is the case. Let's discuss this offline if needed. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:32: std::string near_end; On 2017/04/07 13:24:31, minyue-webrtc wrote: > can add const Done. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:33: std::string far_end; On 2017/04/07 13:24:31, minyue-webrtc wrote: > can add const Done. https://codereview.webrtc.org/2790933002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:35: // RTC_DISALLOW_COPY_AND_ASSIGN(SpeakerOutputFilePaths); On 2017/04/07 13:24:31, minyue-webrtc wrote: > why commented out Sorry, "leftover". Don't need it anymore.
Hi Minyue, I added two missing corrections. The CL is now ready for the second iteration. Alessio
https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:74: if (it != audiotrack_readers_.end()) If this is supposed to be incremental, why doesn't sample_rate_ perhaps contain a valid value? https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:88: LOG(LS_ERROR) << "all the audio tracks should have the same sample rate"; "A"ll and add full stop at the end of the sentence. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:93: LOG(LS_ERROR) << "only mono audio tracks supported"; "O"nly and full stop. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:97: audiotrack_readers_.emplace( Is this emplace allowed? try a few trybots. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:57: int sample_rate() const; can put getter function inline int sample_rate_hz() const { return sample_rate_hz_; } https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:81: int sample_rate_; needs a unit, ie., sample_rate_hz_; https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.cc (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:46: LOG(LS_VERBOSE) << "creating " << near_end_path.pathname(); I'd like full sentence in the log https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:54: std::piecewise_construct, I am not a fan of this, ok, I think I understand now. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:71: WavWriter& near_end() { WavWriter* near_end_wav_writer() https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:74: WavWriter& far_end() { WavWriter* far_end_wav_writer() https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:78: WavWriter near_end_; near_end_wav_writer_ https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:79: WavWriter far_end_; far_end_wav_writer_ https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:137: if (padding_size > 0) { I prefer "!=0": a bit easier to read https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:153: if (padding_size > 0) { != 0 https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:185: for (const auto& speaking_turn : speaking_turns) { I think you may put multiend_call.speaking_turns() in the for() https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:192: &speakers_wav_writers->at( remove & if you return * with near_end() there are other similar places. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.h (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:29: : near_end(std::move(new_near_end)), I am not a fan of move string, is there a benefit?
Hi Minyue, Thanks for your comments! PTAL https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:74: if (it != audiotrack_readers_.end()) On 2017/05/16 15:05:16, minyue-webrtc wrote: > If this is supposed to be incremental, why doesn't sample_rate_ perhaps contain > a valid value? Here I check if the audio track file has already been found before. Hence, audiotrack_readers_ is incremental. No need for the sample rate since all the files must have the same sr. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:88: LOG(LS_ERROR) << "all the audio tracks should have the same sample rate"; On 2017/05/16 15:05:16, minyue-webrtc wrote: > "A"ll and add full stop at the end of the sentence. Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:93: LOG(LS_ERROR) << "only mono audio tracks supported"; On 2017/05/16 15:05:16, minyue-webrtc wrote: > "O"nly and full stop. Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:97: audiotrack_readers_.emplace( On 2017/05/16 15:05:16, minyue-webrtc wrote: > Is this emplace allowed? try a few trybots. No need to try. This line was landed in a previous CL and it didn't fail. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:57: int sample_rate() const; On 2017/05/16 15:05:16, minyue-webrtc wrote: > can put getter function inline > > int sample_rate_hz() const { return sample_rate_hz_; } Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:81: int sample_rate_; On 2017/05/16 15:05:16, minyue-webrtc wrote: > needs a unit, ie., sample_rate_hz_; Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.cc (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:46: LOG(LS_VERBOSE) << "creating " << near_end_path.pathname(); On 2017/05/16 15:05:17, minyue-webrtc wrote: > I'd like full sentence in the log Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:54: std::piecewise_construct, On 2017/05/16 15:05:16, minyue-webrtc wrote: > I am not a fan of this, ok, I think I understand now. Acknowledged. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:74: WavWriter& far_end() { On 2017/05/16 15:05:17, minyue-webrtc wrote: > WavWriter* far_end_wav_writer() Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:78: WavWriter near_end_; On 2017/05/16 15:05:17, minyue-webrtc wrote: > near_end_wav_writer_ Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:79: WavWriter far_end_; On 2017/05/16 15:05:17, minyue-webrtc wrote: > far_end_wav_writer_ Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:137: if (padding_size > 0) { On 2017/05/16 15:05:17, minyue-webrtc wrote: > I prefer "!=0": a bit easier to read Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:153: if (padding_size > 0) { On 2017/05/16 15:05:17, minyue-webrtc wrote: > != 0 Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:185: for (const auto& speaking_turn : speaking_turns) { On 2017/05/16 15:05:16, minyue-webrtc wrote: > I think you may put multiend_call.speaking_turns() in the for() Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:192: &speakers_wav_writers->at( On 2017/05/16 15:05:17, minyue-webrtc wrote: > remove & if you return * with near_end() > > there are other similar places. Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.h (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:29: : near_end(std::move(new_near_end)), On 2017/05/16 15:05:17, minyue-webrtc wrote: > I am not a fan of move string, is there a benefit? Different reviewer, different tastes :) I had a discussion exactly about this some time ago. Karl and Henrik concluded that this is a good way to go. Check at line 38 in https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro...
https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.h (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:29: : near_end(std::move(new_near_end)), On 2017/05/17 12:49:38, AleBzk wrote: > On 2017/05/16 15:05:17, minyue-webrtc wrote: > > I am not a fan of move string, is there a benefit? > > Different reviewer, different tastes :) > > I had a discussion exactly about this some time ago. > Karl and Henrik concluded that this is a good way to go. > > Check at line 38 in > https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... It's more efficient (since you don't have to make a new heap allocation, copy the contents, and free the old heap allocation). And as long as you make sure to not use the moved-from string between the point of the std::move and when it goes out of scope, nothing can go wrong. (If you nevertheless go with not moving, pass the arguments by const& instead.)
https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:97: audiotrack_readers_.emplace( On 2017/05/17 12:49:37, AleBzk wrote: > On 2017/05/16 15:05:16, minyue-webrtc wrote: > > Is this emplace allowed? try a few trybots. > > No need to try. This line was landed in a previous CL and it didn't fail. Acknowledged. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.h (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.h:29: : near_end(std::move(new_near_end)), On 2017/05/18 13:28:49, kwiberg-webrtc wrote: > On 2017/05/17 12:49:38, AleBzk wrote: > > On 2017/05/16 15:05:17, minyue-webrtc wrote: > > > I am not a fan of move string, is there a benefit? > > > > Different reviewer, different tastes :) > > > > I had a discussion exactly about this some time ago. > > Karl and Henrik concluded that this is a good way to go. > > > > Check at line 38 in > > > https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... > > It's more efficient (since you don't have to make a new heap allocation, copy > the contents, and free the old heap allocation). And as long as you make sure to > not use the moved-from string between the point of the std::move and when it > goes out of scope, nothing can go wrong. > > (If you nevertheless go with not moving, pass the arguments by const& instead.) I am concerned about the definition, from the signature SpeakerOutputFilePaths(std::string new_near_end, std::string new_far_end) I won't be able to tell that I will gain efficiency if I call it with SpeakerOutputFilePaths(std::move(str_1), std::move(str_2)) SpeakerOutputFilePaths(std::string&& new_near_end, std::string&& new_far_end) and SpeakerOutputFilePaths(const std::string& new_near_end, const std::string& new_far_end) are more obvious to me. We can have both :)
As discussed offline, let's just get a const&.
lgtm only a nit https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.cc (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:78: WavWriter near_end_; On 2017/05/17 12:49:38, AleBzk wrote: > On 2017/05/16 15:05:17, minyue-webrtc wrote: > > near_end_wav_writer_ > > Done. I'd prefer change the var name to near_end_wav_writer_ https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:79: WavWriter far_end_; On 2017/05/17 12:49:38, AleBzk wrote: > On 2017/05/16 15:05:17, minyue-webrtc wrote: > > far_end_wav_writer_ > > Done. same here
https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/simulator.cc (right): https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:78: WavWriter near_end_; On 2017/05/19 12:46:51, minyue-webrtc wrote: > On 2017/05/17 12:49:38, AleBzk wrote: > > On 2017/05/16 15:05:17, minyue-webrtc wrote: > > > near_end_wav_writer_ > > > > Done. > > I'd prefer change the var name to near_end_wav_writer_ Done. https://codereview.webrtc.org/2790933002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/simulator.cc:79: WavWriter far_end_; On 2017/05/19 12:46:51, minyue-webrtc wrote: > On 2017/05/17 12:49:38, AleBzk wrote: > > On 2017/05/16 15:05:17, minyue-webrtc wrote: > > > far_end_wav_writer_ > > > > Done. > > same here Done.
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2790933002/#ps140001 (title: "fix after merge")
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: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/9482)
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2790933002/#ps160001 (title: "win trybot failure isolated")
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: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/19408)
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2790933002/#ps220001 (title: "UT workaround due to webrtc:7769 bug")
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": 220001, "attempt_start_ts": 1496857326679240,
"parent_rev": "bb28b35922b195971ecf7f76b3bb9e0a03f57f6e", "commit_rev":
"6b648c4697cede14605fd2b89425866eec5f7c79"}
Message was sent while issue was closed.
Description was changed from ========== The simulator puts into action the schedule of speech turns encoded in a MultiEndCall instance. The output is a set of audio track pairs. There is one set for each speaker and each set contains one near-end and one far-end audio track. The tracks are directly written into wav files instead of creating them in memory. To speed up the creation of the output wav files, *all* the source audio tracks (i.e., the atomic speech turns) are pre-loaded. The ConversationalSpeechTest.MultiEndCallSimulator unit test defines a conversational speech sequence and creates two wav files (with pure tones at 440 and 880 Hz) that are used as atomic speech turn tracks. This CL also patches MultiEndCall in order to allow input audio tracks with same sample rate and single channel only. BUG=webrtc:7218 ========== to ========== The simulator puts into action the schedule of speech turns encoded in a MultiEndCall instance. The output is a set of audio track pairs. There is one set for each speaker and each set contains one near-end and one far-end audio track. The tracks are directly written into wav files instead of creating them in memory. To speed up the creation of the output wav files, *all* the source audio tracks (i.e., the atomic speech turns) are pre-loaded. The ConversationalSpeechTest.MultiEndCallSimulator unit test defines a conversational speech sequence and creates two wav files (with pure tones at 440 and 880 Hz) that are used as atomic speech turn tracks. This CL also patches MultiEndCall in order to allow input audio tracks with same sample rate and single channel only. BUG=webrtc:7218 Review-Url: https://codereview.webrtc.org/2790933002 Cr-Commit-Position: refs/heads/master@{#18480} Committed: https://chromium.googlesource.com/external/webrtc/+/6b648c4697cede14605fd2b89... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/external/webrtc/+/6b648c4697cede14605fd2b89...
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.webrtc.org/2925123003/ by charujain@webrtc.org. The reason for reverting is: Compile Error.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
