Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750353002:
Conversational speech tool: timing model with data access. (Closed)
|
Created:
3 years, 9 months ago by AleBzk Modified:
3 years, 9 months ago Reviewers:
kwiberg-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. |
DescriptionConversational speech tool: timing model with data access.
The conversational_speech::Timing class models a list of turns.
Each turn, is identified by a speaker, the audiotrack name, and an offset in milliseconds.
The unit test checks that an instance of Timing is correctly populated and that save/reload leads to the same data.
BUG=webrtc:7218
Review-Url: https://codereview.webrtc.org/2750353002
Cr-Commit-Position: refs/heads/master@{#17346}
Committed: https://chromium.googlesource.com/external/webrtc/+/ce302b82c919182887784eff6e6aca7716c6c9c1
Patch Set 1 #
Total comments: 32
Patch Set 2 : comments from Karl addressed #
Total comments: 26
Patch Set 3 : comments from Karl addressed #Patch Set 4 : minor changes #
Total comments: 6
Patch Set 5 : final comments addressed #Patch Set 6 : minor fix #Patch Set 7 : deps fixed #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 22 (10 generated)
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
See comments below. Most importantly, I don't think you should use unique_ptr here---using Turn directly will work just fine. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:24: typedef conversational_speech::Timing::Turn Turn; I don't think there's ever a reason to use typedef instead of using in new code. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:31: Turn const turns[kNumberOfTurns] = { Don't put a number between the []. That way, it can't be wrong... https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:45: for (Turn turn_ : turns) { Please don't use trailing _ in anything except member variable names. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:48: timing->AppendTurn(std::move(turn)); You don't need to bind the std::unique_ptr<Turn> to a local variable---just pass it directly to AppendTurn. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:79: } To minimize the risk of human error, consider incrementing index in a separate statement at the end. Actually, maybe this would be even better: auto actual_turns = timing->turns(); EXPECT_EQ(turns.size(), actual_turns.size()); for (size_t i = 0; i < turns.size(); ++i) { ...all the EXPECT_EQs go here... } One should try to avoid creative array indexing whenever possible. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:83: std::string temporary_filepath = webrtc::test::TempFilename( const std::string. It's a good habit to use const whenever possible---it helps the reader, since she doesn't have to consider the possibility that the variable is modified below. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:106: Turn* turn = turns[index].get(); You don't need to bind these to local variables. Also, are they both unique_ptrs? In that case, don't use .get()---unique_ptr has a suitable operator==. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/conversational_speech/timing.cc (right): https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:32: Clear(); Not necessary, right? https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:37: // deleted (I assume not). No, unique_ptr will delete the thing it owns---that's pretty much its only job... https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:62: AppendTurn(parse_line_()); Pass line as an argument here, instead of capturing a reference to it in the lambda! As it is, it's very easy to miss this implicit argument. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:79: // return an iterator. This is fine. You could also return an ArrayView<std::unique_ptr<Timing::Turn>> (ArrayView<Timing::Turn> if you follow my other advice). https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:85: } Is this necessary? Callers can do .turns().size(). https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/conversational_speech/timing.h (right): https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.h:26: int offset_) No trailing underscore on non-member variables, please. (IIRC you can give them the same names as the members without running into trouble. But it's probably better to just give them other names.) https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.h:55: std::vector<std::unique_ptr<Turn>> turns_; Turn is a simple value type. Why not just have a std::vector<Turn>? Do you expect that to lead to excessive copying of Turn objects? It doesn't look like that would happen.
Thanks for your comments Karl. I addressed most of them and asked questions for a few ones. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:24: typedef conversational_speech::Timing::Turn Turn; On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > I don't think there's ever a reason to use typedef instead of using in new code. Just wanted to make an alias. Should I remove both? Or can I leave the first? https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:31: Turn const turns[kNumberOfTurns] = { On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > Don't put a number between the []. That way, it can't be wrong... This was intentional even if it may look strange. The constant is used in TEST(ConversationalSpeechTest, TimingInstanceRead) to check the expected size. In this way, if an entry is added in turns without updating kNumberOfTurns, a compilation error arises. I find this better than having a test that fails. But please, just tell me what is better according to the internal standards. Third option, using sizeof(turns) / sizeof(*turns). https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:45: for (Turn turn_ : turns) { On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > Please don't use trailing _ in anything except member variable names. Done. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:48: timing->AppendTurn(std::move(turn)); On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > You don't need to bind the std::unique_ptr<Turn> to a local variable---just pass > it directly to AppendTurn. Done. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:79: } On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > To minimize the risk of human error, consider incrementing index in a separate > statement at the end. > > Actually, maybe this would be even better: > > auto actual_turns = timing->turns(); > EXPECT_EQ(turns.size(), actual_turns.size()); > for (size_t i = 0; i < turns.size(); ++i) { > ...all the EXPECT_EQs go here... > } > > One should try to avoid creative array indexing whenever possible. Done. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:83: std::string temporary_filepath = webrtc::test::TempFilename( On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > const std::string. It's a good habit to use const whenever possible---it helps > the reader, since she doesn't have to consider the possibility that the variable > is modified below. Done. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:106: Turn* turn = turns[index].get(); On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > You don't need to bind these to local variables. Also, are they both > unique_ptrs? In that case, don't use .get()---unique_ptr has a suitable > operator==. Done. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/conversational_speech/timing.cc (right): https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:32: Clear(); On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > Not necessary, right? Acknowledged. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:37: // deleted (I assume not). On 2017/03/16 14:10:09, kwiberg-webrtc wrote: > No, unique_ptr will delete the thing it owns---that's pretty much its only > job... Acknowledged. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:62: AppendTurn(parse_line_()); On 2017/03/16 14:10:09, kwiberg-webrtc wrote: > Pass line as an argument here, instead of capturing a reference to it in the > lambda! As it is, it's very easy to miss this implicit argument. Done. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:79: // return an iterator. On 2017/03/16 14:10:09, kwiberg-webrtc wrote: > This is fine. You could also return an ArrayView<std::unique_ptr<Timing::Turn>> > (ArrayView<Timing::Turn> if you follow my other advice). I'm new with ArrayView, I guess you refer to the one defined in //webrtc/base/array_view.h https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:85: } On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > Is this necessary? Callers can do .turns().size(). Removed https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/conversational_speech/timing.h (right): https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.h:26: int offset_) On 2017/03/16 14:10:09, kwiberg-webrtc wrote: > No trailing underscore on non-member variables, please. (IIRC you can give them > the same names as the members without running into trouble. But it's probably > better to just give them other names.) Done. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.h:55: std::vector<std::unique_ptr<Turn>> turns_; On 2017/03/16 14:10:09, kwiberg-webrtc wrote: > Turn is a simple value type. Why not just have a std::vector<Turn>? Do you > expect that to lead to excessive copying of Turn objects? It doesn't look like > that would happen. True. At this moment copying is not an issue. I expect <20 Turn objects and I don't expect that they will ever become more complex than a struct with 3 members (2 strings and one integer). https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:81: I added a new constructor for Timing to familiarize with initializer lists. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/timing.h (right): https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.h:39: Timing(std::initializer_list<Turn> il); I added this to familiarize with initializer list.
https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:24: typedef conversational_speech::Timing::Turn Turn; On 2017/03/20 14:25:27, AleBzk wrote: > On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > > I don't think there's ever a reason to use typedef instead of using in new > code. > > Just wanted to make an alias. > Should I remove both? Or can I leave the first? Sorry, I should probably have put quotes around "using" to make that easier to parse. You can do exactly what you do, but use "using" instead of "typedef". https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:31: Turn const turns[kNumberOfTurns] = { On 2017/03/20 14:25:28, AleBzk wrote: > On 2017/03/16 14:10:08, kwiberg-webrtc wrote: > > Don't put a number between the []. That way, it can't be wrong... > > This was intentional even if it may look strange. > The constant is used in TEST(ConversationalSpeechTest, TimingInstanceRead) to > check the expected size. In this way, if an entry is added in turns without > updating kNumberOfTurns, a compilation error arises. I find this better than > having a test that fails. > > But please, just tell me what is better according to the internal standards. > > Third option, using sizeof(turns) / sizeof(*turns). Third option. Use [] without a number, and use arraysize(turns) when you need the size. But often you don't, because range for loops and std::begin/std::end work with arrays. https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/conversational_speech/timing.cc (right): https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:79: // return an iterator. On 2017/03/20 14:25:28, AleBzk wrote: > On 2017/03/16 14:10:09, kwiberg-webrtc wrote: > > This is fine. You could also return an > ArrayView<std::unique_ptr<Timing::Turn>> > > (ArrayView<Timing::Turn> if you follow my other advice). > > I'm new with ArrayView, I guess you refer to the one defined in > //webrtc/base/array_view.h Yes. (I wrote it, so if you have questions or complaints, direct them my way...) https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/conversational_speech/timing.h (right): https://codereview.webrtc.org/2750353002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/conversational_speech/timing.h:55: std::vector<std::unique_ptr<Turn>> turns_; On 2017/03/20 14:25:28, AleBzk wrote: > On 2017/03/16 14:10:09, kwiberg-webrtc wrote: > > Turn is a simple value type. Why not just have a std::vector<Turn>? Do you > > expect that to lead to excessive copying of Turn objects? It doesn't look like > > that would happen. > > True. At this moment copying is not an issue. I expect <20 Turn objects and I > don't expect that they will ever become more complex than a struct with 3 > members (2 strings and one integer). OK, definitely just use a std::vector<Turn> then. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:46: for (Turn source_turn : turns) { Almost always use const& (or just &, if you need mutability) for the loop variable in ranged for loops; otherwise you make a copy. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:50: source_turn.offset)); Why not just timing->AppendTurn(source_turn); ? https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:51: } Hmm. Use the new initializer_list constructor here? Or replace it with a constructor that accepts an ArrayView<const Turn>? https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:79: EXPECT_EQ(actual_timing.turns().size(), kNumberOfTurns); "kNumberOfTurns" isn't much use here. Just use a literal 6. Also, the convention is to put the expected value in the first argument position when you use {EXPECT,ASSERT,CHECK,DCHECK}_EQ. I think you're consistently doing the opposite (understandable, since that's the natural order...). https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:81: On 2017/03/20 14:25:28, AleBzk wrote: > I added a new constructor for Timing to familiarize with initializer lists. Hmm. Is it useful enough to pay for the extra complexity? https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:96: EXPECT_EQ(actual_turn.offset, expected_turn.offset); Should Turn have an operator==? Or does it already?---you seem to be using it below. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:120: EXPECT_TRUE(expected_turns[index] == actual_turns[index]) EXPECT_EQ? https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/timing.cc (right): https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:35: } Can't you pass the initializer_list directly to turns_'s constructor? https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:42: // deleted (I assume not). Comment no longer applicable. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/timing.h (right): https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.h:32: bool operator ==(const Turn &b) const; No space before == https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.h:49: void Load(const std::string& timing_filepath); Would it make sense to turn this into a constructor? Or a static function that returns a new Timing? https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.h:58: }; Hmm. Now that you no longer use unique_ptr, Timing doesn't really *do* anything. You could replace it with free functions to save to and load from a file, and just store the data in a std::vector<Turn>.
Everything addressed, now much simpler. Compare PS#2 to PS#4. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:46: for (Turn source_turn : turns) { On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > Almost always use const& (or just &, if you need mutability) for the loop > variable in ranged for loops; otherwise you make a copy. Done. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:50: source_turn.offset)); On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > Why not just > > timing->AppendTurn(source_turn); > > ? Not applying anymore. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:51: } On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > Hmm. Use the new initializer_list constructor here? Or replace it with a > constructor that accepts an ArrayView<const Turn>? Not applying anymore. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:79: EXPECT_EQ(actual_timing.turns().size(), kNumberOfTurns); On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > "kNumberOfTurns" isn't much use here. Just use a literal 6. > > Also, the convention is to put the expected value in the first argument position > when you use {EXPECT,ASSERT,CHECK,DCHECK}_EQ. I think you're consistently doing > the opposite (understandable, since that's the natural order...). Done. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:81: On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > On 2017/03/20 14:25:28, AleBzk wrote: > > I added a new constructor for Timing to familiarize with initializer lists. > > Hmm. Is it useful enough to pay for the extra complexity? I don't need it anymore due to the refactoring. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:96: EXPECT_EQ(actual_turn.offset, expected_turn.offset); On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > Should Turn have an operator==? Or does it already?---you seem to be using it > below. Right, however I removed this test due to the refactoring. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:120: EXPECT_TRUE(expected_turns[index] == actual_turns[index]) On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > EXPECT_EQ? Done. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/timing.cc (right): https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:35: } On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > Can't you pass the initializer_list directly to turns_'s constructor? Acknowledged. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:42: // deleted (I assume not). On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > Comment no longer applicable. Acknowledged. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/timing.h (right): https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.h:32: bool operator ==(const Turn &b) const; On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > No space before == Done. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.h:49: void Load(const std::string& timing_filepath); On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > Would it make sense to turn this into a constructor? Or a static function that > returns a new Timing? Right. I'll go for the constructor way. https://codereview.webrtc.org/2750353002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.h:58: }; On 2017/03/21 22:35:15, kwiberg-webrtc wrote: > Hmm. Now that you no longer use unique_ptr, Timing doesn't really *do* anything. > You could replace it with free functions to save to and load from a file, and > just store the data in a std::vector<Turn>. Done.
lgtm, but see remaining nits https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:57: } This test should have a different name now. Also, you can probably remove it, since it's rather easy to see that expected_timing has size >0. https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/timing.cc (right): https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:32: auto parse_line_ = [](const std::string& line) { No trailing underscore, since this isn't a member variable. https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/timing.h (right): https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.h:38: const std::vector<Turn>& timing); The second argument could be ArrayView<const Turn>. Then the caller could hold the Turns in any array, not just a std::vector.
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2750353002/#ps80001 (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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24109) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22906) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2750353002/#ps120001 (title: "deps fixed")
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: linux_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)
The CQ bit was checked by alessiob@webrtc.org
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": 120001, "attempt_start_ts": 1490195753042620,
"parent_rev": "f505e83dc133aca1d6c22a510827415463a809a7", "commit_rev":
"ce302b82c919182887784eff6e6aca7716c6c9c1"}
Message was sent while issue was closed.
Description was changed from ========== Conversational speech tool: timing model with data access. The conversational_speech::Timing class models a list of turns. Each turn, is identified by a speaker, the audiotrack name, and an offset in milliseconds. The unit test checks that an instance of Timing is correctly populated and that save/reload leads to the same data. BUG=webrtc:7218 ========== to ========== Conversational speech tool: timing model with data access. The conversational_speech::Timing class models a list of turns. Each turn, is identified by a speaker, the audiotrack name, and an offset in milliseconds. The unit test checks that an instance of Timing is correctly populated and that save/reload leads to the same data. BUG=webrtc:7218 Review-Url: https://codereview.webrtc.org/2750353002 Cr-Commit-Position: refs/heads/master@{#17346} Committed: https://chromium.googlesource.com/external/webrtc/+/ce302b82c919182887784eff6... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/ce302b82c919182887784eff6...
Message was sent while issue was closed.
https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc (right): https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:57: } On 2017/03/22 12:06:39, kwiberg-webrtc wrote: > This test should have a different name now. Also, you can probably remove it, > since it's rather easy to see that expected_timing has size >0. Done. https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/timing.cc (right): https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.cc:32: auto parse_line_ = [](const std::string& line) { On 2017/03/22 12:06:39, kwiberg-webrtc wrote: > No trailing underscore, since this isn't a member variable. Done. https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/conversational_speech/timing.h (right): https://codereview.webrtc.org/2750353002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/conversational_speech/timing.h:38: const std::vector<Turn>& timing); On 2017/03/22 12:06:39, kwiberg-webrtc wrote: > The second argument could be ArrayView<const Turn>. Then the caller could hold > the Turns in any array, not just a std::vector. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
