Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781573002:
Conversational Speech tool, MultiEndCall::CheckTiming() and tests (Closed)
|
Created:
3 years, 8 months ago by AleBzk Modified:
3 years, 8 months ago Reviewers:
hlundin-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. |
DescriptionMultiEndCall::CheckTiming() verifies that a set of audio tracks and timing information is valid to simulate conversational speech. Unordered turns are rejected. Self cross-talk and cross-talk with 3 or more speakers are not permitted since it would require mixing at the simulation step.
This CL includes extensive tests to match accept or reject decisions on several different timing setups. The setups are simulated using mocks (by far more light-weight than using actual timing and audio track files).
The client code, the unit tests in this case, passes information about the fake audio tracks to MockWavReaderFactory. MockWavReader instances are then created using the parameters defined in the client code. To improve the readability of the tests, generator_unittest.cc includes a docstring explaining how each MultiEndCallSetup* test is documented.
Run tests as follows:
$ out/Default/modules_unittests --gtest_filter=ConversationalSpeechTest.*
BUG=webrtc:7218
Review-Url: https://codereview.webrtc.org/2781573002
Cr-Commit-Position: refs/heads/master@{#17592}
Committed: https://chromium.googlesource.com/external/webrtc/+/8d23c050f2d25e625f16b98e5cb6ada42529b23e
Patch Set 1 #Patch Set 2 : all test pass #Patch Set 3 : minor fixes #Patch Set 4 : rebase #Patch Set 5 : rebase #
Total comments: 1
Patch Set 6 : merge #Patch Set 7 : refactoring in view of coming CLs #Patch Set 8 : final refactoring #
Total comments: 62
Patch Set 9 : 1st round comments addressed #Patch Set 10 : comments from Henrik addressed #
Total comments: 1
Patch Set 11 : final comments addressed #Patch Set 12 : missing include to get std::back_inserter working on win targets #Dependent Patchsets: Messages
Total messages: 23 (9 generated)
Description was changed from ========== MultiEndCall::CheckTiming() stub ready unit tests implemented ad-hoc audio track parameters for MockWavReader BUG=webrtc:7218 ========== to ========== MultiEndCall::CheckTiming() verifies that a set of audio tracks and timing information is valid to simulate conversational speech. Unordered turns are rejected. Self cross-talk and cross-talk with 3 or more speakers are not permitted since it would require mixing at the simulation step. This CL includes extensive tests to match accept or reject decisions on several different timing setups. The setups are simulated using mocks (by far more light-weight than using actual timing and audio track files). The client code, the unit tests in this case, passes information about the fake audio tracks to MockWavReaderFactory. MockWavReader instances are then created using the parameters defined in the client code. To improve the readability of the tests, generator_unittest.cc includes a docstring explaining how each MultiEndCallSetup* test is documented. Run tests as follows: $ out/Default/modules_unittests --gtest_filter=ConversationalSpeechTest.* BUG=webrtc:7218 ==========
alessiob@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Hi Henrik, This CL follows https://codereview.webrtc.org/2761853002. Alessio
https://codereview.webrtc.org/2781573002/diff/70001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2781573002/diff/70001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:89: {"t1000", kMockWavReaderFactoryParams1000ms}, t300, t500 and t1000 will be used as fake audio track file names to let the factory automatically recall kMockWavReaderFactoryParams300ms, kMockWavReaderFactoryParams500ms and kMockWavReaderFactoryParams1000ms respectively.
This is very nice. I have a few comments inline. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:75: const int kDefaultSampleRate = 48000; constexpr https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:76: const std::map<std::string, const MockWavReaderFactory::Params> constexpr? https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:83: // Default arguments for MockWavReaderFactory ctor. This comment should go above the preceding map, right? https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:84: const MockWavReaderFactory::Params& kDefaultMockWavReaderFactoryParams = constexpr? https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:98: rtc::LogMessage::LogToDebug(rtc::LS_VERBOSE); I don't think you want to do all of this verbose logging in the unit test by default. The output gets very chatty. I'd prefer if you skip this (and also revert to not having a test fixture). When needed for local debugging, a developer can always add this to the individual TEST. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:149: TEST_F(ConversationalSpeechTest, MultiEndCallSetupFirstOffsetNonNegative) { "NonNegative"? Seems negative to me. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:157: EXPECT_CALL(*mock_wavreader_factory, Create(testing::_)).Times(1); I suggest you do using testing::_ to make all of these places more compact. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:168: const std::size_t expected_duration = kDefaultSampleRate; Throughout the file: const -> constexpr when possible. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader.cc:13: #include "webrtc/test/gmock.h" No need for this; it is already present in the h-file. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc:35: audiotrack_names_params_.insert(params.begin(), params.end()); You should be able to use the copy constructor, and put in the initializer list: audiotrack_names_params_(params) Then you can also make audiotrack_names_params_ const. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc:44: auto it = audiotrack_names_params_.find(audiotrack_file_path.filename()); const auto it? https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:82: std::size_t begin; size_t Here and below. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:87: return ms * sr / 1000; I'd recommend rtc::CheckedDivExact(sr, 1000) https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:112: RTC_CHECK(it != audiotrack_readers_.end()) RTC_CHECK_NE https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:125: if (offset_samples < 0 && -offset_samples > int( static_cast<int>(last_turn.end - last_turn.begin) https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:145: speaking_turn_indices[turn.speaker_name].push_back(turn_index); You are relying on an implicit assumption that turn_index is equal to the current length of speaking_turns_, right? DCHECK that. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:158: for (const std::string& speaker_name : speaker_names_) { The speaking_turn_indices variable is only used for detecting self cross-talk. I think you should be able to do that without speaking_turn_indices. for (const std::string& speaker_name : speaker_names_) { std::vector<SpeakingTurn> speaking_turns_for_name; // Copy all turns for this speaker to new vector. std::copy_if(speaking_turns_.begin(), speaking_turns_.end(), speaking_turns_for_name.begin(), [&speaker_name](const SpeakingTurn& st){ return st.speaker_name == speaker_name; }); // Check for overlap between adjacent elements. auto overlap = std::adjacent_find(speaking_turns_for_name.begin(), speaking_turns_for_name.end(), [](const SpeakingTurn& a, const SpeakingTurn& b) { return a.end > b.begin; }); if (overlap != speaking_turns_for_name.end()) { LOG()...; return false; } } https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:161: if (DetectSelfCrossTalk(speaking_turn_indices[speaker_name])) { It is a bit tricky to use the map::[] operator here. In case something went wrong previously, and speaking_turn_indices contains no entry with key speaker_name, the [] operator will create such an entry. I'd rather see the code crash and burn in that case. Either you can use the map::at() method, but that is defined to throw an exception if the entry is not found; we don't use exceptions. Or, you can use the map::find() method, and DCHECK you didn't get past-then-end iterator as answer. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:37: std::size_t new_begin, std::size_t new_end) We tend to use size_t instead of std::size_t. Include e.g., stddef.h for declaration. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:38: : speaker_name(std::move(new_speaker_name)), In not an expert in move semantics, but this looks weird to me. Pruning it down to a simple example, you are doing something like this: class B; struct A { A(B new_b) : b(std::move(new_b) {} B b; } // Using the ctor: B my_b; A my_a(my_b); If I'm not mistaken, this will result in the ctor parameter b being a copy, which then is moved from. I think you can make the ctor parameter a const ref, and do the copy inside the ctor instead: A(const B& new_b) : b(new_b) {} This will also result in one copy, but saves the unnecessary (and confusing) move operation. If you really want to move from my_b, you will have to declare the ctor parameter as using &&: A(B&& new_b) : b(std::move(new_b)) {} and call it with A my_a(std::move(my_b)); // my_b is invalid after this. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:56: std::size_t total_duration_samples() const; size_t https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:73: const std::vector<std::size_t>& speaking_turn_indices) const; size_t https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:82: std::size_t total_duration_samples_; size_t
Drive-by explanation. :-) https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:38: : speaker_name(std::move(new_speaker_name)), On 2017/04/06 08:10:04, hlundin-webrtc wrote: > In not an expert in move semantics, but this looks weird to me. Pruning it down > to a simple example, you are doing something like this: > > class B; > > struct A { > A(B new_b) : b(std::move(new_b) {} > > B b; > } > > // Using the ctor: > B my_b; > A my_a(my_b); > > If I'm not mistaken, this will result in the ctor parameter b being a copy, > which then is moved from. Well, yes. It's up to the caller to create the B that's passed to A's constructor, and it can do that with the copy or move constructor. The following will result in two moves and no copy: B my_b; A my_a(std::move(my_b)); > I think you can make the ctor parameter a const ref, and do the copy inside the > ctor instead: > A(const B& new_b) : b(new_b) {} > This will also result in one copy, but saves the unnecessary (and confusing) > move operation. That's a valid way to do it, but as you say it'll always do a copy. I think the way the CL currently does it is better. > If you really want to move from my_b, you will have to declare the ctor > parameter as using &&: > A(B&& new_b) : b(std::move(new_b)) {} > and call it with > A my_a(std::move(my_b)); // my_b is invalid after this. That'll change two things, compared to what the CL does now: (1) result in a total of just one move instead of two, and (2) force the caller to do std::move or something equivalent---copying won't be an option.
https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/BUILD.gn (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/BUILD.gn:70: "//webrtc/test:test_support", tldr: replace '//webrtc/base' and test with '../../<maybe a few ../ more>/base' and 'test'. Avoid absolute imports that start with '//webrtc'. Our code is imported into chrome, but in there '//webrtc' is actually '//third_party/webrtc'. It's possible to test that chrome still compiles before submitting. For that, do git cl try -m tryserver.chromium.linux -b linux_chromium_rel_ng git cl try -m tryserver.chromium.mac -b mac_chromium_rel_ng -b ios-device git cl try -m tryserver.chromium.android -b android_compile_dbg -b linux_android_rel_ng
Thanks a lot for your comments. There are still a couple of unsolved points, I added the details to make a final decision. Alessio https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/BUILD.gn (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/BUILD.gn:70: "//webrtc/test:test_support", On 2017/04/06 08:37:04, aleloi wrote: > tldr: replace '//webrtc/base' and test with '../../<maybe a few ../ more>/base' > and 'test'. > > Avoid absolute imports that start with '//webrtc'. Our code is imported into > chrome, but in there '//webrtc' is actually '//third_party/webrtc'. > > It's possible to test that chrome still compiles before submitting. For that, do > > git cl try -m tryserver.chromium.linux -b linux_chromium_rel_ng > git cl try -m tryserver.chromium.mac -b mac_chromium_rel_ng -b ios-device > git cl try -m tryserver.chromium.android -b android_compile_dbg -b > linux_android_rel_ng Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:75: const int kDefaultSampleRate = 48000; On 2017/04/06 08:10:03, hlundin-webrtc wrote: > constexpr Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:76: const std::map<std::string, const MockWavReaderFactory::Params> On 2017/04/06 08:10:03, hlundin-webrtc wrote: > constexpr? Nope (error: constexpr variable cannot have non-literal type). https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:83: // Default arguments for MockWavReaderFactory ctor. On 2017/04/06 08:10:03, hlundin-webrtc wrote: > This comment should go above the preceding map, right? Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:84: const MockWavReaderFactory::Params& kDefaultMockWavReaderFactoryParams = On 2017/04/06 08:10:03, hlundin-webrtc wrote: > constexpr? Nope because I can't use constexpr with the const std::map defined above. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:98: rtc::LogMessage::LogToDebug(rtc::LS_VERBOSE); On 2017/04/06 08:10:03, hlundin-webrtc wrote: > I don't think you want to do all of this verbose logging in the unit test by > default. The output gets very chatty. I'd prefer if you skip this (and also > revert to not having a test fixture). When needed for local debugging, a > developer can always add this to the individual TEST. You won't see any log unless you add "--logs true" when running modules_unittests. What about reverting to tests without a fixture and no logging with a final CL? I still have to fix this CL and a dependent one (https://codereview.webrtc.org/2790933002). It could be useful to keep fixture and logging until the whole conversational tool is finalized. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:149: TEST_F(ConversationalSpeechTest, MultiEndCallSetupFirstOffsetNonNegative) { On 2017/04/06 08:10:03, hlundin-webrtc wrote: > "NonNegative"? Seems negative to me. Sorry. I should have called it RejectNonNegative, but I'll go for Negative. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:157: EXPECT_CALL(*mock_wavreader_factory, Create(testing::_)).Times(1); On 2017/04/06 08:10:03, hlundin-webrtc wrote: > I suggest you do > using testing::_ > to make all of these places more compact. Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:168: const std::size_t expected_duration = kDefaultSampleRate; On 2017/04/06 08:10:03, hlundin-webrtc wrote: > Throughout the file: const -> constexpr when possible. Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader.cc:13: #include "webrtc/test/gmock.h" On 2017/04/06 08:10:03, hlundin-webrtc wrote: > No need for this; it is already present in the h-file. Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc:35: audiotrack_names_params_.insert(params.begin(), params.end()); On 2017/04/06 08:10:04, hlundin-webrtc wrote: > You should be able to use the copy constructor, and put in the initializer list: > audiotrack_names_params_(params) > Then you can also make audiotrack_names_params_ const. If I'm correct, we have two options: 1) instead of calling the MockWavReaderFactory(const Params& default_params) ctor in the initializer list of the second ctor, we duplicate the code of the former in the latter and then yes, we can do as you suggest. 2) we leave everything as it is to avoid code duplication Personally, I prefer the second. But there might be other ways of using the copy ctor while still calling MockWavReaderFactory(const Params& default_params). https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc:44: auto it = audiotrack_names_params_.find(audiotrack_file_path.filename()); On 2017/04/06 08:10:04, hlundin-webrtc wrote: > const auto it? Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:82: std::size_t begin; On 2017/04/06 08:10:04, hlundin-webrtc wrote: > size_t > Here and below. Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:87: return ms * sr / 1000; On 2017/04/06 08:10:04, hlundin-webrtc wrote: > I'd recommend rtc::CheckedDivExact(sr, 1000) If I do that, the tool won't work if the sampling rate is not an integer multiple of 1k (e.g., it'd crash with 22050 and 44100 Hz). Ceiling, floor or rounding are all fine here; so implicit casting is ok. WDYT? https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:112: RTC_CHECK(it != audiotrack_readers_.end()) On 2017/04/06 08:10:04, hlundin-webrtc wrote: > RTC_CHECK_NE RTC_CHECK_NE(it, audiotrack_readers_.end()) raises tons of compiling errors. I guess that can be fixed by casting, but IMHO I'd find that less readable. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:125: if (offset_samples < 0 && -offset_samples > int( On 2017/04/06 08:10:04, hlundin-webrtc wrote: > static_cast<int>(last_turn.end - last_turn.begin) Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:145: speaking_turn_indices[turn.speaker_name].push_back(turn_index); On 2017/04/06 08:10:04, hlundin-webrtc wrote: > You are relying on an implicit assumption that turn_index is equal to the > current length of speaking_turns_, right? DCHECK that. Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:158: for (const std::string& speaker_name : speaker_names_) { On 2017/04/06 08:10:04, hlundin-webrtc wrote: > The speaking_turn_indices variable is only used for detecting self cross-talk. I > think you should be able to do that without speaking_turn_indices. > > for (const std::string& speaker_name : speaker_names_) { > std::vector<SpeakingTurn> speaking_turns_for_name; > // Copy all turns for this speaker to new vector. > std::copy_if(speaking_turns_.begin(), speaking_turns_.end(), > speaking_turns_for_name.begin(), > [&speaker_name](const SpeakingTurn& st){ > return st.speaker_name == speaker_name; }); > // Check for overlap between adjacent elements. > auto overlap = std::adjacent_find(speaking_turns_for_name.begin(), > speaking_turns_for_name.end(), > [](const SpeakingTurn& a, const SpeakingTurn& b) { > return a.end > b.begin; }); > if (overlap != speaking_turns_for_name.end()) { > LOG()...; > return false; > } > } Cool! Happy to learn about std::copy_if and std::adjacent_find. And great usage of lambdas. Thanks! I did something similar initially, but then I preferred to avoid waste of memory. That's why I eventually went for speaking_turn_indices (more lightweight). Since this tool will never handle millions of speaking turns, I will stick to your snippet (more readable). https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:161: if (DetectSelfCrossTalk(speaking_turn_indices[speaker_name])) { On 2017/04/06 08:10:04, hlundin-webrtc wrote: > It is a bit tricky to use the map::[] operator here. In case something went > wrong previously, and speaking_turn_indices contains no entry with key > speaker_name, the [] operator will create such an entry. I'd rather see the code > crash and burn in that case. > > Either you can use the map::at() method, but that is defined to throw an > exception if the entry is not found; we don't use exceptions. Or, you can use > the map::find() method, and DCHECK you didn't get past-then-end iterator as > answer. I'll go the way you suggested in your previous comment, but thanks anyway for this comment. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:37: std::size_t new_begin, std::size_t new_end) On 2017/04/06 08:10:04, hlundin-webrtc wrote: > We tend to use size_t instead of std::size_t. Include e.g., stddef.h for > declaration. Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:38: : speaker_name(std::move(new_speaker_name)), Thanks for your comments on this point. Before answering inline below, two important notes: - as reported in the comment before SpeakingTurn is defined, these objects are created via std::vector::emplace_back() - the original code in multiend_call.cc does not use std::move (search "speaking_turns_.emplace_back" in MultiEndCall::CheckTiming()) and *should not* On 2017/04/06 08:35:32, kwiberg-webrtc wrote: > On 2017/04/06 08:10:04, hlundin-webrtc wrote: > > In not an expert in move semantics, but this looks weird to me. Pruning it > down > > to a simple example, you are doing something like this: > > > > class B; > > > > struct A { > > A(B new_b) : b(std::move(new_b) {} > > > > B b; > > } > > > > // Using the ctor: > > B my_b; > > A my_a(my_b); > > > > If I'm not mistaken, this will result in the ctor parameter b being a copy, > > which then is moved from. > > Well, yes. It's up to the caller to create the B that's passed to A's > constructor, and it can do that with the copy or move constructor. The following > will result in two moves and no copy: > > B my_b; > A my_a(std::move(my_b)); > > > I think you can make the ctor parameter a const ref, and do the copy inside > the > > ctor instead: > > A(const B& new_b) : b(new_b) {} > > This will also result in one copy, but saves the unnecessary (and confusing) > > move operation. > > That's a valid way to do it, but as you say it'll always do a copy. I think the > way the CL currently does it is better. > > > If you really want to move from my_b, you will have to declare the ctor > > parameter as using &&: > > A(B&& new_b) : b(std::move(new_b)) {} > > and call it with > > A my_a(std::move(my_b)); // my_b is invalid after this. > > That'll change two things, compared to what the CL does now: (1) result in a > total of just one move instead of two, and (2) force the caller to do std::move > or something equivalent---copying won't be an option. We can't go for this last option otherwise, if I'm correct, I have to use std::move while passing the ctor args in emplace_back. But what I pass there is used afterwards, hence std::move is not possible. The only alternative is going for const std::string& in the SpeakingTurn ctor as Henrik suggested. Result: for now I didn't change anything because we have a tie. Karl finds the current CL better than const std::string&, but Henrik finds better the latter for readability. Do the details I shared above about the caller (namely, emplace_back) help in making a final decision? https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:56: std::size_t total_duration_samples() const; On 2017/04/06 08:10:04, hlundin-webrtc wrote: > size_t Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:73: const std::vector<std::size_t>& speaking_turn_indices) const; On 2017/04/06 08:10:04, hlundin-webrtc wrote: > size_t Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:82: std::size_t total_duration_samples_; On 2017/04/06 08:10:04, hlundin-webrtc wrote: > size_t Done.
https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:76: const std::map<std::string, const MockWavReaderFactory::Params> On 2017/04/06 16:42:41, AleBzk wrote: > On 2017/04/06 08:10:03, hlundin-webrtc wrote: > > constexpr? > > Nope (error: constexpr variable cannot have non-literal type). Acknowledged. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:84: const MockWavReaderFactory::Params& kDefaultMockWavReaderFactoryParams = On 2017/04/06 16:42:41, AleBzk wrote: > On 2017/04/06 08:10:03, hlundin-webrtc wrote: > > constexpr? > > Nope because I can't use constexpr with the const std::map defined above. Acknowledged. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:98: rtc::LogMessage::LogToDebug(rtc::LS_VERBOSE); On 2017/04/06 16:42:41, AleBzk wrote: > On 2017/04/06 08:10:03, hlundin-webrtc wrote: > > I don't think you want to do all of this verbose logging in the unit test by > > default. The output gets very chatty. I'd prefer if you skip this (and also > > revert to not having a test fixture). When needed for local debugging, a > > developer can always add this to the individual TEST. > > You won't see any log unless you add "--logs true" when running > modules_unittests. > What about reverting to tests without a fixture and no logging with a final CL? > I still have to fix this CL and a dependent one > (https://codereview.webrtc.org/2790933002). > It could be useful to keep fixture and logging until the whole conversational > tool is finalized. OK. Add a TODO in the code about his. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc:35: audiotrack_names_params_.insert(params.begin(), params.end()); On 2017/04/06 16:42:42, AleBzk wrote: > On 2017/04/06 08:10:04, hlundin-webrtc wrote: > > You should be able to use the copy constructor, and put in the initializer > list: > > audiotrack_names_params_(params) > > Then you can also make audiotrack_names_params_ const. > > If I'm correct, we have two options: > 1) instead of calling the MockWavReaderFactory(const Params& default_params) > ctor in the initializer list of the second ctor, we duplicate the code of the > former in the latter and then yes, we can do as you suggest. > 2) we leave everything as it is to avoid code duplication > > Personally, I prefer the second. > But there might be other ways of using the copy ctor while still calling > MockWavReaderFactory(const Params& default_params). I wonder if we are talking about different things. I mean you could write this ctor as: MockWavReaderFactory::MockWavReaderFactory( const Params& default_params, const std::map<std::string, const Params>& params) : MockWavReaderFactory(default_params) audiotrack_names_params_(params) {} That is, use the copy ctor of audiotrack_names_params_ instead of calling the insert() method. Unless I'm mistaken, you should then also be able to make audiotrack_names_params_ const declared, since you are only setting it in an initializer list. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:87: return ms * sr / 1000; On 2017/04/06 16:42:42, AleBzk wrote: > On 2017/04/06 08:10:04, hlundin-webrtc wrote: > > I'd recommend rtc::CheckedDivExact(sr, 1000) > > If I do that, the tool won't work if the sampling rate is not an integer > multiple of 1k (e.g., it'd crash with 22050 and 44100 Hz). Ceiling, floor or > rounding are all fine here; so implicit casting is ok. > > WDYT? Oh, the tool should be able to handle other rates than 8, 16, 32, 48? That changes everything. Keep this as is, then, if you are convinced that the truncation is fine. Please, add a comment that truncation may happen. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:112: RTC_CHECK(it != audiotrack_readers_.end()) On 2017/04/06 16:42:42, AleBzk wrote: > On 2017/04/06 08:10:04, hlundin-webrtc wrote: > > RTC_CHECK_NE > > RTC_CHECK_NE(it, audiotrack_readers_.end()) raises tons of compiling errors. I > guess that can be fixed by casting, but IMHO I'd find that less readable. Hmm. Boring. Keep this as is then. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:38: : speaker_name(std::move(new_speaker_name)), On 2017/04/06 16:42:42, AleBzk wrote: > Thanks for your comments on this point. > Before answering inline below, two important notes: > - as reported in the comment before SpeakingTurn is defined, these objects are > created via std::vector::emplace_back() > - the original code in multiend_call.cc does not use std::move (search > "speaking_turns_.emplace_back" in MultiEndCall::CheckTiming()) and *should not* > > On 2017/04/06 08:35:32, kwiberg-webrtc wrote: > > On 2017/04/06 08:10:04, hlundin-webrtc wrote: > > > In not an expert in move semantics, but this looks weird to me. Pruning it > > down > > > to a simple example, you are doing something like this: > > > > > > class B; > > > > > > struct A { > > > A(B new_b) : b(std::move(new_b) {} > > > > > > B b; > > > } > > > > > > // Using the ctor: > > > B my_b; > > > A my_a(my_b); > > > > > > If I'm not mistaken, this will result in the ctor parameter b being a copy, > > > which then is moved from. > > > > Well, yes. It's up to the caller to create the B that's passed to A's > > constructor, and it can do that with the copy or move constructor. The > following > > will result in two moves and no copy: > > > > B my_b; > > A my_a(std::move(my_b)); > > > > > I think you can make the ctor parameter a const ref, and do the copy inside > > the > > > ctor instead: > > > A(const B& new_b) : b(new_b) {} > > > This will also result in one copy, but saves the unnecessary (and confusing) > > > move operation. > > > > That's a valid way to do it, but as you say it'll always do a copy. I think > the > > way the CL currently does it is better. > > > > > If you really want to move from my_b, you will have to declare the ctor > > > parameter as using &&: > > > A(B&& new_b) : b(std::move(new_b)) {} > > > and call it with > > > A my_a(std::move(my_b)); // my_b is invalid after this. > > > > That'll change two things, compared to what the CL does now: (1) result in a > > total of just one move instead of two, and (2) force the caller to do > std::move > > or something equivalent---copying won't be an option. > > We can't go for this last option otherwise, if I'm correct, I have to use > std::move while passing the ctor args in emplace_back. But what I pass there is > used afterwards, hence std::move is not possible. > The only alternative is going for const std::string& in the SpeakingTurn ctor as > Henrik suggested. > > Result: for now I didn't change anything because we have a tie. Karl finds the > current CL better than const std::string&, but Henrik finds better the latter > for readability. Do the details I shared above about the caller (namely, > emplace_back) help in making a final decision? You can keep this as is. I was mainly concerned that you *thought* you were using move semantics all the way, avoiding copying completely, but were in fact not.
Everything addressed. Thanks for your additional comments. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:98: rtc::LogMessage::LogToDebug(rtc::LS_VERBOSE); On 2017/04/07 10:24:09, hlundin-webrtc wrote: > On 2017/04/06 16:42:41, AleBzk wrote: > > On 2017/04/06 08:10:03, hlundin-webrtc wrote: > > > I don't think you want to do all of this verbose logging in the unit test by > > > default. The output gets very chatty. I'd prefer if you skip this (and also > > > revert to not having a test fixture). When needed for local debugging, a > > > developer can always add this to the individual TEST. > > > > You won't see any log unless you add "--logs true" when running > > modules_unittests. > > What about reverting to tests without a fixture and no logging with a final > CL? > > I still have to fix this CL and a dependent one > > (https://codereview.webrtc.org/2790933002). > > It could be useful to keep fixture and logging until the whole conversational > > tool is finalized. > > OK. Add a TODO in the code about his. Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc:35: audiotrack_names_params_.insert(params.begin(), params.end()); On 2017/04/07 10:24:09, hlundin-webrtc wrote: > On 2017/04/06 16:42:42, AleBzk wrote: > > On 2017/04/06 08:10:04, hlundin-webrtc wrote: > > > You should be able to use the copy constructor, and put in the initializer > > list: > > > audiotrack_names_params_(params) > > > Then you can also make audiotrack_names_params_ const. > > > > If I'm correct, we have two options: > > 1) instead of calling the MockWavReaderFactory(const Params& default_params) > > ctor in the initializer list of the second ctor, we duplicate the code of the > > former in the latter and then yes, we can do as you suggest. > > 2) we leave everything as it is to avoid code duplication > > > > Personally, I prefer the second. > > But there might be other ways of using the copy ctor while still calling > > MockWavReaderFactory(const Params& default_params). > > I wonder if we are talking about different things. I mean you could write this > ctor as: > > MockWavReaderFactory::MockWavReaderFactory( > const Params& default_params, > const std::map<std::string, const Params>& params) > : MockWavReaderFactory(default_params) > audiotrack_names_params_(params) {} > > That is, use the copy ctor of audiotrack_names_params_ instead of calling the > insert() method. Unless I'm mistaken, you should then also be able to make > audiotrack_names_params_ const declared, since you are only setting it in an > initializer list. Yes, then I understood correctly your point. I already tried that and it fails with " error: an initializer for a delegating constructor must appear alone". However, I fixed by removing the ctor with a single arg and adding a default value. Now audiotrack_names_params_is const ref. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:87: return ms * sr / 1000; On 2017/04/07 10:24:09, hlundin-webrtc wrote: > On 2017/04/06 16:42:42, AleBzk wrote: > > On 2017/04/06 08:10:04, hlundin-webrtc wrote: > > > I'd recommend rtc::CheckedDivExact(sr, 1000) > > > > If I do that, the tool won't work if the sampling rate is not an integer > > multiple of 1k (e.g., it'd crash with 22050 and 44100 Hz). Ceiling, floor or > > rounding are all fine here; so implicit casting is ok. > > > > WDYT? > > Oh, the tool should be able to handle other rates than 8, 16, 32, 48? That > changes everything. Keep this as is, then, if you are convinced that the > truncation is fine. Please, add a comment that truncation may happen. Done. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc:112: RTC_CHECK(it != audiotrack_readers_.end()) On 2017/04/07 10:24:09, hlundin-webrtc wrote: > On 2017/04/06 16:42:42, AleBzk wrote: > > On 2017/04/06 08:10:04, hlundin-webrtc wrote: > > > RTC_CHECK_NE > > > > RTC_CHECK_NE(it, audiotrack_readers_.end()) raises tons of compiling errors. I > > guess that can be fixed by casting, but IMHO I'd find that less readable. > > Hmm. Boring. Keep this as is then. Acknowledged. https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h:38: : speaker_name(std::move(new_speaker_name)), On 2017/04/07 10:24:09, hlundin-webrtc wrote: > On 2017/04/06 16:42:42, AleBzk wrote: > > Thanks for your comments on this point. > > Before answering inline below, two important notes: > > - as reported in the comment before SpeakingTurn is defined, these objects are > > created via std::vector::emplace_back() > > - the original code in multiend_call.cc does not use std::move (search > > "speaking_turns_.emplace_back" in MultiEndCall::CheckTiming()) and *should > not* > > > > On 2017/04/06 08:35:32, kwiberg-webrtc wrote: > > > On 2017/04/06 08:10:04, hlundin-webrtc wrote: > > > > In not an expert in move semantics, but this looks weird to me. Pruning it > > > down > > > > to a simple example, you are doing something like this: > > > > > > > > class B; > > > > > > > > struct A { > > > > A(B new_b) : b(std::move(new_b) {} > > > > > > > > B b; > > > > } > > > > > > > > // Using the ctor: > > > > B my_b; > > > > A my_a(my_b); > > > > > > > > If I'm not mistaken, this will result in the ctor parameter b being a > copy, > > > > which then is moved from. > > > > > > Well, yes. It's up to the caller to create the B that's passed to A's > > > constructor, and it can do that with the copy or move constructor. The > > following > > > will result in two moves and no copy: > > > > > > B my_b; > > > A my_a(std::move(my_b)); > > > > > > > I think you can make the ctor parameter a const ref, and do the copy > inside > > > the > > > > ctor instead: > > > > A(const B& new_b) : b(new_b) {} > > > > This will also result in one copy, but saves the unnecessary (and > confusing) > > > > move operation. > > > > > > That's a valid way to do it, but as you say it'll always do a copy. I think > > the > > > way the CL currently does it is better. > > > > > > > If you really want to move from my_b, you will have to declare the ctor > > > > parameter as using &&: > > > > A(B&& new_b) : b(std::move(new_b)) {} > > > > and call it with > > > > A my_a(std::move(my_b)); // my_b is invalid after this. > > > > > > That'll change two things, compared to what the CL does now: (1) result in a > > > total of just one move instead of two, and (2) force the caller to do > > std::move > > > or something equivalent---copying won't be an option. > > > > We can't go for this last option otherwise, if I'm correct, I have to use > > std::move while passing the ctor args in emplace_back. But what I pass there > is > > used afterwards, hence std::move is not possible. > > The only alternative is going for const std::string& in the SpeakingTurn ctor > as > > Henrik suggested. > > > > Result: for now I didn't change anything because we have a tie. Karl finds the > > current CL better than const std::string&, but Henrik finds better the latter > > for readability. Do the details I shared above about the caller (namely, > > emplace_back) help in making a final decision? > > You can keep this as is. I was mainly concerned that you *thought* you were > using move semantics all the way, avoiding copying completely, but were in fact > not. Acknowledged.
LGTM with suggestions (as discussed offline, too). https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc (right): https://codereview.webrtc.org/2781573002/diff/130001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.cc:35: audiotrack_names_params_.insert(params.begin(), params.end()); On 2017/04/07 11:37:06, AleBzk wrote: > On 2017/04/07 10:24:09, hlundin-webrtc wrote: > > On 2017/04/06 16:42:42, AleBzk wrote: > > > On 2017/04/06 08:10:04, hlundin-webrtc wrote: > > > > You should be able to use the copy constructor, and put in the initializer > > > list: > > > > audiotrack_names_params_(params) > > > > Then you can also make audiotrack_names_params_ const. > > > > > > If I'm correct, we have two options: > > > 1) instead of calling the MockWavReaderFactory(const Params& default_params) > > > ctor in the initializer list of the second ctor, we duplicate the code of > the > > > former in the latter and then yes, we can do as you suggest. > > > 2) we leave everything as it is to avoid code duplication > > > > > > Personally, I prefer the second. > > > But there might be other ways of using the copy ctor while still calling > > > MockWavReaderFactory(const Params& default_params). > > > > I wonder if we are talking about different things. I mean you could write this > > ctor as: > > > > MockWavReaderFactory::MockWavReaderFactory( > > const Params& default_params, > > const std::map<std::string, const Params>& params) > > : MockWavReaderFactory(default_params) > > audiotrack_names_params_(params) {} > > > > That is, use the copy ctor of audiotrack_names_params_ instead of calling the > > insert() method. Unless I'm mistaken, you should then also be able to make > > audiotrack_names_params_ const declared, since you are only setting it in an > > initializer list. > > Yes, then I understood correctly your point. > I already tried that and it fails with " error: an initializer for a delegating > constructor must appear alone". > > However, I fixed by removing the ctor with a single arg and adding a default > value. > Now audiotrack_names_params_is const ref. Do like this: Let the member be a mutable variable (not a ref). Use copy ctor here. https://codereview.webrtc.org/2781573002/diff/170001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.h (right): https://codereview.webrtc.org/2781573002/diff/170001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/conversational_speech/mock_wavreader_factory.h:37: params = std::map<std::string, const Params>{}); Remove default arg and replace the extra ctor instead.
Done.
The CQ bit was checked by alessiob@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/2781573002/#ps190001 (title: "final comments addressed")
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_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/12609)
The CQ bit was checked by alessiob@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/2781573002/#ps210001 (title: "missing include to get std::back_inserter working on win targets")
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": 210001, "attempt_start_ts": 1491590373176440,
"parent_rev": "292084c3765d9f3ee406ca2ec86eae206b540053", "commit_rev":
"8d23c050f2d25e625f16b98e5cb6ada42529b23e"}
Message was sent while issue was closed.
Description was changed from ========== MultiEndCall::CheckTiming() verifies that a set of audio tracks and timing information is valid to simulate conversational speech. Unordered turns are rejected. Self cross-talk and cross-talk with 3 or more speakers are not permitted since it would require mixing at the simulation step. This CL includes extensive tests to match accept or reject decisions on several different timing setups. The setups are simulated using mocks (by far more light-weight than using actual timing and audio track files). The client code, the unit tests in this case, passes information about the fake audio tracks to MockWavReaderFactory. MockWavReader instances are then created using the parameters defined in the client code. To improve the readability of the tests, generator_unittest.cc includes a docstring explaining how each MultiEndCallSetup* test is documented. Run tests as follows: $ out/Default/modules_unittests --gtest_filter=ConversationalSpeechTest.* BUG=webrtc:7218 ========== to ========== MultiEndCall::CheckTiming() verifies that a set of audio tracks and timing information is valid to simulate conversational speech. Unordered turns are rejected. Self cross-talk and cross-talk with 3 or more speakers are not permitted since it would require mixing at the simulation step. This CL includes extensive tests to match accept or reject decisions on several different timing setups. The setups are simulated using mocks (by far more light-weight than using actual timing and audio track files). The client code, the unit tests in this case, passes information about the fake audio tracks to MockWavReaderFactory. MockWavReader instances are then created using the parameters defined in the client code. To improve the readability of the tests, generator_unittest.cc includes a docstring explaining how each MultiEndCallSetup* test is documented. Run tests as follows: $ out/Default/modules_unittests --gtest_filter=ConversationalSpeechTest.* BUG=webrtc:7218 Review-Url: https://codereview.webrtc.org/2781573002 Cr-Commit-Position: refs/heads/master@{#17592} Committed: https://chromium.googlesource.com/external/webrtc/+/8d23c050f2d25e625f16b98e5... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:210001) as https://chromium.googlesource.com/external/webrtc/+/8d23c050f2d25e625f16b98e5... |
