Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2711473003:
R/PLR calculation - time-based window (Closed)
|
Created:
3 years, 10 months ago by elad.alon Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, henrika_webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionR/PLR calculation - time-based window
BUG=webrtc:7058
Review-Url: https://codereview.webrtc.org/2711473003
Cr-Commit-Position: refs/heads/master@{#17251}
Committed: https://chromium.googlesource.com/external/webrtc/+/3f9b12d0564fb8e5c640b1cf114a8b763b1ca49d
Patch Set 1 #Patch Set 2 : Fuzzer still not done. #Patch Set 3 : Fuzzer done. #Patch Set 4 : Individual probabilities explicitly stated, not cumulative. #
Total comments: 2
Patch Set 5 : . #
Total comments: 22
Patch Set 6 : CR response. #
Total comments: 2
Patch Set 7 : CR response #Patch Set 8 : Make 1ms/10ms a bit clearer. #
Total comments: 69
Patch Set 9 : Fuzzer fix. #
Total comments: 1
Patch Set 10 : Response to Minyue's CR #
Total comments: 3
Patch Set 11 : CR response #
Total comments: 4
Patch Set 12 : . #Patch Set 13 : . #Patch Set 14 : Comment turned into TODO. #Patch Set 15 : CR response #Patch Set 16 : Revert emplace_hint; not all compilers support. #Patch Set 17 : More compiler nits. #
Dependent Patchsets: Messages
Total messages: 52 (15 generated)
elad.alon@webrtc.org changed reviewers: + michaelt@webrtc.org, minyue@webrtc.org, sprang@webrtc.org, stefan@webrtc.org
Ready for review.
Description was changed from ========== R/PLR calculation - time-based window BUG=webrtc:7058 ========== to ========== R/PLR calculation - time-based window BUG=webrtc:7058 ==========
elad.alon@webrtc.org changed reviewers: + pbos@webrtc.org
Added pbos for the fuzzer. Minyue, are there any changes you'd like?
https://codereview.webrtc.org/2711473003/diff/60001/webrtc/test/fuzzers/trans... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/60001/webrtc/test/fuzzers/trans... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:263: int64_t time_ms = Clock::GetRealTimeClock()->TimeInMilliseconds(); Read a 32bit value from the fuzzed data instead, if you use realtime the results are not reproducible.
https://codereview.webrtc.org/2711473003/diff/60001/webrtc/test/fuzzers/trans... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/60001/webrtc/test/fuzzers/trans... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:263: int64_t time_ms = Clock::GetRealTimeClock()->TimeInMilliseconds(); On 2017/03/03 18:37:34, pbos-webrtc wrote: > Read a 32bit value from the fuzzed data instead, if you use realtime the results > are not reproducible. Will do.
https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:70: if (packet_status_window_.size() > 0) { Can you move the if into the RTC_DCHECK so we will not have a empty if in release mode ? https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:94: std::make_pair(seq_num, sent_packet)); Use: "packet_status_window_.emplace_hint(packet_status_window_.end(),std::make_pair(seq_num, SentPacket(send_time_ms, PacketStatus::Unacked)));" To construct SentPacket direct in the map. So we can save a copy. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:103: const auto& fb_vector = feedback.GetStatusVector(); Unrelated to this CL. But can you rename "fb_vector" to "feedback_statuses" or something like that. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:113: continue; Can we RTC_DCHECK this ? https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:117: const PacketStatus new_packet_status = I think "acked_packet_status" would be more clear than "new_packet_status" https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:134: void TransportFeedbackPacketLossTracker::RecordPacketStatus( How about rename this function to "UpdatedPacketStatus" and "new_packet_status" to "packet_status" ? https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:39: tracker->OnPacketAdded(sequence_number, time_ms_++); "time_ms_++" seams not really obvious. Would prefer to add something like "send_interval_ms" to the interface. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:40: if (validate_all) { All the test run on my PC in 200ms so i don't with this switch is necessary. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:272: // Up to the maximum time-span retained (first + 9 * 100ms). Is the max window in this case not 9ms ?
https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:70: if (packet_status_window_.size() > 0) { On 2017/03/07 09:15:26, michaelt wrote: > Can you move the if into the RTC_DCHECK so we will not have a empty if in > release mode ? That would require RTC_DCHECK(x || y), which I hope the other reviewers would be fine with. Me, I'm also for it. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:94: std::make_pair(seq_num, sent_packet)); On 2017/03/07 09:15:26, michaelt wrote: > Use: > "packet_status_window_.emplace_hint(packet_status_window_.end(),std::make_pair(seq_num, > SentPacket(send_time_ms, PacketStatus::Unacked)));" > > To construct SentPacket direct in the map. So we can save a copy. Done. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:103: const auto& fb_vector = feedback.GetStatusVector(); On 2017/03/07 09:15:26, michaelt wrote: > Unrelated to this CL. But can you rename > "fb_vector" to "feedback_statuses" or something like that. > Done. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:113: continue; On 2017/03/07 09:15:26, michaelt wrote: > Can we RTC_DCHECK this ? We can't; it's not an erroneous case. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:117: const PacketStatus new_packet_status = On 2017/03/07 09:15:26, michaelt wrote: > I think "acked_packet_status" would be more clear than "new_packet_status" I might as well just call it "packet_status" then. Let's go with that. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:134: void TransportFeedbackPacketLossTracker::RecordPacketStatus( On 2017/03/07 09:15:26, michaelt wrote: > How about rename this function to "UpdatedPacketStatus" and "new_packet_status" > to "packet_status" ? Thinking out loud: Pro - we really sometimes overwrite an already recorded (acked) status. Con - this is for recording the acked status, which normally happens only once. Updating from Lost to Received is an edge case. I'll go with UpdatePacketStatus(it, new_status). It makes it clearer down the line that the variable new_packet_status refers to the status which we write in, rather than some intermediate variable. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:39: tracker->OnPacketAdded(sequence_number, time_ms_++); On 2017/03/07 09:15:26, michaelt wrote: > "time_ms_++" seams not really obvious. > Would prefer to add something like "send_interval_ms" to the interface. > I've added that, but I am not 100% sure I agree that adding a default-value for a parameter, when nobody use a non-default value, is the way to go. But I'm also not 100% sure it should just be constexpr either, so we'll go with that. Edit: I've made use of it in one test. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:40: if (validate_all) { On 2017/03/07 09:15:26, michaelt wrote: > All the test run on my PC in 200ms so i don't with this switch is necessary. They run quickly *because* of that. Try removing the if, you'll see how MaxUnackedPackets takes an unexpectedly long time. If you've run this locally before, you might have run with --gtest_filter and didn't get the parameterized tests to run. Make sure you look out for that one test I've mentioned. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:272: // Up to the maximum time-span retained (first + 9 * 100ms). On 2017/03/07 09:15:26, michaelt wrote: > Is the max window in this case not 9ms ? The comment became out of date when I changed some things about the test; I'll fix. Thanks for catching. Come to think of it, would be nice to have this use the new parameter for send time interval, thereby making it more apparent that the 9 corresponds to time, not number of packets.
https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:39: tracker->OnPacketAdded(sequence_number, time_ms_++); I would prefer to have it without a default coded in the function and use instead a constexpr and pass that for each call to SendPackets. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:40: if (validate_all) { On 2017/03/07 11:55:19, elad.alon wrote: > On 2017/03/07 09:15:26, michaelt wrote: > > All the test run on my PC in 200ms so i don't with this switch is necessary. > > They run quickly *because* of that. Try removing the if, you'll see how > MaxUnackedPackets takes an unexpectedly long time. If you've run this locally > before, you might have run with --gtest_filter and didn't get the parameterized > tests to run. Make sure you look out for that one test I've mentioned. Acknowledged. https://codereview.webrtc.org/2711473003/diff/100001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/100001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:219: tracker.OnPacketAdded(base_ + 0, time_ms); Can we use SendPackets with a interval of Zero here ?
https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:39: tracker->OnPacketAdded(sequence_number, time_ms_++); On 2017/03/07 12:33:12, michaelt wrote: > I would prefer to have it without a default coded in the function and use > instead a constexpr and pass that for each call to SendPackets. > I'm a bit split about this - hard for me to decide if it's clearer to have callers of the function provide less parameters, or to have everything explicitly stated. I took your suggestion. https://codereview.webrtc.org/2711473003/diff/100001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/100001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:219: tracker.OnPacketAdded(base_ + 0, time_ms); On 2017/03/07 12:33:13, michaelt wrote: > Can we use SendPackets with a interval of Zero here ? Yeah, that's now possible.
lgtm
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:261: int64_t time_ms = FuzzInput<uint32_t>(&data, &size); Looks like this is reusing data from Setup? time_ms should be a separate part of the data segment right (not tied to setup data)?
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:261: int64_t time_ms = FuzzInput<uint32_t>(&data, &size); On 2017/03/07 15:13:59, pbos-webrtc wrote: > Looks like this is reusing data from Setup? time_ms should be a separate part of > the data segment right (not tied to setup data)? I'm not sure I understand. The function FuzzInput() moves |data|, so that data is not reused.
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:261: int64_t time_ms = FuzzInput<uint32_t>(&data, &size); On 2017/03/07 15:35:07, elad.alon wrote: > On 2017/03/07 15:13:59, pbos-webrtc wrote: > > Looks like this is reusing data from Setup? time_ms should be a separate part > of > > the data segment right (not tied to setup data)? > > I'm not sure I understand. The function FuzzInput() moves |data|, so that data > is not reused. Gotcha. I wasn't around when FuzzInput was a thing. How does FuzzInput work if size here is zero, or < sizeof(uint32_t) ?
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:261: int64_t time_ms = FuzzInput<uint32_t>(&data, &size); On 2017/03/07 15:39:33, pbos-webrtc wrote: > On 2017/03/07 15:35:07, elad.alon wrote: > > On 2017/03/07 15:13:59, pbos-webrtc wrote: > > > Looks like this is reusing data from Setup? time_ms should be a separate > part > > of > > > the data segment right (not tied to setup data)? > > > > I'm not sure I understand. The function FuzzInput() moves |data|, so that data > > is not reused. > > Gotcha. I wasn't around when FuzzInput was a thing. How does FuzzInput work if > size here is zero, or < sizeof(uint32_t) ? E.g. may_continue is not updated. Is that not a problem because FuzzPacketSendBlock will fail as size has become zero after it (even if size was 3 before FuzzInput)? I would assume FuzzInput would return success/failure. Is that not the case?
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:261: int64_t time_ms = FuzzInput<uint32_t>(&data, &size); On 2017/03/07 15:41:31, pbos-webrtc wrote: > On 2017/03/07 15:39:33, pbos-webrtc wrote: > > On 2017/03/07 15:35:07, elad.alon wrote: > > > On 2017/03/07 15:13:59, pbos-webrtc wrote: > > > > Looks like this is reusing data from Setup? time_ms should be a separate > > part > > > of > > > > the data segment right (not tied to setup data)? > > > > > > I'm not sure I understand. The function FuzzInput() moves |data|, so that > data > > > is not reused. > > > > Gotcha. I wasn't around when FuzzInput was a thing. How does FuzzInput work if > > size here is zero, or < sizeof(uint32_t) ? > > E.g. may_continue is not updated. Is that not a problem because > FuzzPacketSendBlock will fail as size has become zero after it (even if size was > 3 before FuzzInput)? I would assume FuzzInput would return success/failure. Is > that not the case? It fails. We must always check |size| is big enough before calling FuzzInput(), which I see I've neglected to do this time. Thanks for catching!
lgtm for fuzzers/ https://codereview.webrtc.org/2711473003/diff/160001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/160001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:261: if (size < sizeof(uint32_t)) { Consider (in a follow-up CL) making FuzzInput return success/fail and returning the uint32_t value through an input argument instead: bool success = FuzzInput(&time_ms, &data, &size) for instance.
Nice and clean CL in general. I have a few comments and I may need a bit more time on the UT and fuzzer. https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2711473003/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:70: if (packet_status_window_.size() > 0) { On 2017/03/07 11:55:18, elad.alon wrote: > On 2017/03/07 09:15:26, michaelt wrote: > > Can you move the if into the RTC_DCHECK so we will not have a empty if in > > release mode ? > > That would require RTC_DCHECK(x || y), which I hope the other reviewers would be > fine with. Me, I'm also for it. yes https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:168: it->second.send_time_ms - ref_packet_status_->second.send_time_ms > the data structure cannot guarantee that, for any seq_num1 > seq_num2, sent_time_1 > sent_time_2. is this a concern? https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:27: // * |max_window_size_ms| is the time-span of the window's acked part. This is correct but a bit hard to understand, how about // |max_window_size_ms|: We count up to |max_window_size_ms| from the sent time of the latest acked packet for the calculation of the metrics. the max diff in sequence number is an implementation detail (can we get rid of it BTW?) and does not need to mention here, IMO. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:66: void Wait(int64_t time_delta_ms) { TimeAdvance https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:110: if (expected_plr && plr) { no need for EXPECT_EQ(static_cast<bool> and if just EXPECT_EQ(expected_plr, plr) https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:116: if (expected_rplr && rplr) { same here
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:168: it->second.send_time_ms - ref_packet_status_->second.send_time_ms > On 2017/03/07 16:47:05, minyue-webrtc wrote: > the data structure cannot guarantee that, for any seq_num1 > seq_num2, > sent_time_1 > sent_time_2. is this a concern? I think it's best to not think about the sequence numbers here, because they could potentially wrap-around in confusing ways - DTX could mean that, even when we only sent p1 and p2, and in that order, sequence_of(p1) == 300 and sequence_of(p2) == 299. And any other order, really. But since we don't use them here, it's not a problem. What I rely on is that time never moves backwards (though it can fail to actually move forward because of clock resolution; we have a unit test covering that). Note: OnPacketAdded() starts with an RTC_DCHECK for time not moving backwards. So we know that when iterating on the data-structure from oldest to newest, the timestamp (which do not wrap-around) could only increase. That's enough for the logic here. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:27: // * |max_window_size_ms| is the time-span of the window's acked part. On 2017/03/07 16:47:05, minyue-webrtc wrote: > This is correct but a bit hard to understand, how about > > // |max_window_size_ms|: We count up to |max_window_size_ms| from the sent > time of the latest acked packet for the calculation of the metrics. > > the max diff in sequence number is an implementation detail (can we get rid of > it BTW?) and does not need to mention here, IMO. Okay, I'll take your phrasing. I think you're right, 0x8000 is probably just a confusing tidbit for someone trying to figure out which values to pass to the ctor when they instantiate the class. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:66: void Wait(int64_t time_delta_ms) { On 2017/03/07 16:47:05, minyue-webrtc wrote: > TimeAdvance I agree Wait() could be improved. How about AdvanceClock()? https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:110: if (expected_plr && plr) { On 2017/03/07 16:47:05, minyue-webrtc wrote: > no need for > > EXPECT_EQ(static_cast<bool> > and if > just > > EXPECT_EQ(expected_plr, plr) This is intentional - it yields much clearer error messages (see comment above the code in question to that effect, btw). Please refer to: https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... I think the example I've given there makes a very compelling case for keeping things like that. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:116: if (expected_rplr && rplr) { On 2017/03/07 16:47:05, minyue-webrtc wrote: > same here Same explanation. :-)
https://codereview.webrtc.org/2711473003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2711473003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:86: while (!packet_status_window_.empty() && Since now the window size is time based, should we make this time based as well ?
a few more, and will continue after a short while https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:171: } else if (*size < sizeof(uint16_t)) { I think we can start with a new if here, since 168 as a special case and it returns. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:55: void SendPackets(TransportFeedbackPacketLossTracker* tracker, why renamed from SendPacketRange, anyways, I like the new name https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:66: void Wait(int64_t time_delta_ms) { On 2017/03/07 17:17:33, elad.alon wrote: > On 2017/03/07 16:47:05, minyue-webrtc wrote: > > TimeAdvance > > I agree Wait() could be improved. How about AdvanceClock()? ok https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:110: if (expected_plr && plr) { On 2017/03/07 17:17:33, elad.alon wrote: > On 2017/03/07 16:47:05, minyue-webrtc wrote: > > no need for > > > > EXPECT_EQ(static_cast<bool> > > and if > > just > > > > EXPECT_EQ(expected_plr, plr) > > This is intentional - it yields much clearer error messages (see comment above > the code in question to that effect, btw). Please refer to: > https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... > I think the example I've given there makes a very compelling case for keeping > things like that. Sorry. I missed the old comment. Yes, I agree with that. But I think that might be improved in rtc::Optional. For now, how about just adding more message. say EXPECT_EQ(expected_plr, plr) << expected_plr.value_or(-1.0) << " vs " << plr.value_or(-1.0); https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:174: SendPackets(&tracker, base_, 3, 10); I think SendPackets can just use a default sent interval, because it looks like that it is always 10, and other values does not give more value. Yes, I see a 0, but you can use explicit addPacket there. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:213: TEST_P(TransportFeedbackPacketLossTrackerTest, CloseClockReads) { CloseClockReads -> SameSentTime https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:248: TEST_P(TransportFeedbackPacketLossTrackerTest, DifferentPacketLengthSanity) { Sounds very specific to packet length, which is not necessarily known to the packet loss tracker. How about DifferentSentIntervals? https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:264: // The window retain information up until the timestamps exceed the configured retains until -> to the timestamps -> sent times that configured maximum -> the max window size. Then -> remove https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:267: TEST_P(TransportFeedbackPacketLossTrackerTest, MaxWindowSize) { I think the test can be made simpler. How about add a packet -> advance time -> check that old packet is out of window. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:279: // After the maximum time-span, older entries discarded to accommodate discarded -> are discarded
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:171: } else if (*size < sizeof(uint16_t)) { On 2017/03/08 09:12:25, minyue-webrtc(OOO_until_Mar19) wrote: > I think we can start with a new if here, since 168 as a special case and it > returns. IMHO, clearer this way - a code structure where exactly one of several cases returns. (You've mentioned before that I can ignore nits. Let me know if you feel strongly about this one, though.) https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:55: void SendPackets(TransportFeedbackPacketLossTracker* tracker, On 2017/03/08 09:12:25, minyue-webrtc(OOO_until_Mar19) wrote: > why renamed from SendPacketRange, anyways, I like the new name 1. I am confused - do you favor the change or the name before it? 2. I am, naturally, for the new name. I think "Range" is misleading when we use "base and sequence length" rather than "base and last elements". https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:110: if (expected_plr && plr) { On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/07 17:17:33, elad.alon wrote: > > On 2017/03/07 16:47:05, minyue-webrtc wrote: > > > no need for > > > > > > EXPECT_EQ(static_cast<bool> > > > and if > > > just > > > > > > EXPECT_EQ(expected_plr, plr) > > > > This is intentional - it yields much clearer error messages (see comment above > > the code in question to that effect, btw). Please refer to: > > > https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... > > I think the example I've given there makes a very compelling case for keeping > > things like that. > > Sorry. I missed the old comment. Yes, I agree with that. But I think that might > be improved in rtc::Optional. For now, how about just adding more message. say > > EXPECT_EQ(expected_plr, plr) << expected_plr.value_or(-1.0) << " vs " << > plr.value_or(-1.0); Good idea - I'll implement that in a separate CL. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:174: SendPackets(&tracker, base_, 3, 10); On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > I think SendPackets can just use a default sent interval, because it looks like > that it is always 10, and other values does not give more value. > > Yes, I see a 0, but you can use explicit addPacket there. Earlier CL had a default value, then I changed that based on discussion with Michael. For Michael, the main selling point for passing value explicitly was a general preference for that (IIRC). For me, the main selling point is that if we have a function with *multiple* default values, and their types are not distinct enough, it's possible to accidentally pass an integer and end up landing it in the boolean, or vice versa. (Case in point - |send_time_interval_ms| and |validate_all|.) https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:213: TEST_P(TransportFeedbackPacketLossTrackerTest, CloseClockReads) { On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > CloseClockReads -> SameSentTime Done. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:248: TEST_P(TransportFeedbackPacketLossTrackerTest, DifferentPacketLengthSanity) { On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > Sounds very specific to packet length, which is not necessarily known to the > packet loss tracker. > > How about DifferentSentIntervals? I want to make it clear that the tracker correctly handles different frame lengths, regardless of DTX, etc. That is, to prove that the trackers doesn't *need* to know about the frame length. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:264: // The window retain information up until the timestamps exceed the configured On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > retains > > until -> to > > the timestamps -> sent times that > > configured maximum -> the max window size. > > Then -> remove Done. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:267: TEST_P(TransportFeedbackPacketLossTrackerTest, MaxWindowSize) { On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > I think the test can be made simpler. How about > > add a packet -> advance time -> check that old packet is out of window. One packet will *not* be out of window in this suggested scenario, by design. We only move the window when we get new acked packets. Reminder - RTT would otherwise have to be taken into account, etc., etc... :-) https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:279: // After the maximum time-span, older entries discarded to accommodate On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > discarded -> are discarded Done. https://codereview.webrtc.org/2711473003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2711473003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:86: while (!packet_status_window_.empty() && On 2017/03/08 07:19:16, michaelt wrote: > Since now the window size is time based, should we make this time based as well > ? Multiple streams mean the TWCC sequence number can still be incremented while the current stream is inactive (DTX, video on/off by demand, etc.). This means that without this protection, we would have been able to see a case where seq_num=13 is immediately followed by seq_num=12, meaning we'd have a window that looks like this = [13, 12]. Then, suppose the *other* stream sends a packet with seq_num=13. To this stream, it would look like an ACK that belongs to it. Important to note: The timestamp of *sending* is regarded upon *ack reception*, not before. This is intentional.
https://codereview.webrtc.org/2711473003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2711473003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:86: while (!packet_status_window_.empty() && On 2017/03/08 10:24:07, elad.alon wrote: > On 2017/03/08 07:19:16, michaelt wrote: > > Since now the window size is time based, should we make this time based as > well > > ? > > Multiple streams mean the TWCC sequence number can still be incremented while > the current stream is inactive (DTX, video on/off by demand, etc.). This means > that without this protection, we would have been able to see a case where > seq_num=13 is immediately followed by seq_num=12, meaning we'd have a window > that looks like this = [13, 12]. Then, suppose the *other* stream sends a packet > with seq_num=13. To this stream, it would look like an ACK that belongs to it. > > Important to note: The timestamp of *sending* is regarded upon *ack reception*, > not before. This is intentional. Acknowledged.
elad.alon@webrtc.org changed reviewers: + solenberg@chromium.org
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:110: if (expected_plr && plr) { On 2017/03/08 10:24:06, elad.alon wrote: > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > On 2017/03/07 17:17:33, elad.alon wrote: > > > On 2017/03/07 16:47:05, minyue-webrtc wrote: > > > > no need for > > > > > > > > EXPECT_EQ(static_cast<bool> > > > > and if > > > > just > > > > > > > > EXPECT_EQ(expected_plr, plr) > > > > > > This is intentional - it yields much clearer error messages (see comment > above > > > the code in question to that effect, btw). Please refer to: > > > > > > https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... > > > I think the example I've given there makes a very compelling case for > keeping > > > things like that. > > > > Sorry. I missed the old comment. Yes, I agree with that. But I think that > might > > be improved in rtc::Optional. For now, how about just adding more message. say > > > > EXPECT_EQ(expected_plr, plr) << expected_plr.value_or(-1.0) << " vs " << > > plr.value_or(-1.0); > > Good idea - I'll implement that in a separate CL. Sorry, now that I've come back to this, meaning to improve rtc::Optional, I see that you've suggested .value_or(). IMHO, this would not be an improvement, as -1.0 is a misleading theoretically-possible value, rather than indicating that the rtc::Optional is unset. That is to say, true/false are not values that an rtc::Optional<float> could take on, so an error message which features them make it immediately clear that the problem is with being set/unset, rather than somehow actually reaching the value -1.0. So I'd rather keep it as is.
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
rs lgtm - only checked style; suggest you let Stefan or Språng verify the logic. https://codereview.webrtc.org/2711473003/diff/200001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/200001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:151: struct probability_distribution { ProbabilityDistribution https://codereview.webrtc.org/2711473003/diff/200001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2711473003/diff/200001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:168: it->second.send_time_ms - ref_packet_status_->second.send_time_ms > Use () around the difference here, to make the precedence obvious
https://codereview.webrtc.org/2711473003/diff/200001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2711473003/diff/200001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:151: struct probability_distribution { On 2017/03/08 20:58:16, the sun wrote: > ProbabilityDistribution Done. https://codereview.webrtc.org/2711473003/diff/200001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2711473003/diff/200001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:168: it->second.send_time_ms - ref_packet_status_->second.send_time_ms > On 2017/03/08 20:58:16, the sun wrote: > Use () around the difference here, to make the precedence obvious Done. (Assumed difference only, not comparison related to difference. Let me know if I assumed wrong.)
Sorry for the delay. Now comes my remaining comments. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:55: void SendPackets(TransportFeedbackPacketLossTracker* tracker, On 2017/03/08 10:24:07, elad.alon wrote: > On 2017/03/08 09:12:25, minyue-webrtc(OOO_until_Mar19) wrote: > > why renamed from SendPacketRange, anyways, I like the new name > > 1. I am confused - do you favor the change or the name before it? > 2. I am, naturally, for the new name. I think "Range" is misleading when we use > "base and sequence length" rather than "base and last elements". Refactoring is generally fine in every CL. But it may make the reviewer think whether it is related to the main purpose of the CL. So it would be good to add a comment to say "this is refactoring" when you send out a CL. This case is in fact a good refactoring. Go ahead. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:110: if (expected_plr && plr) { On 2017/03/08 10:24:06, elad.alon wrote: > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > On 2017/03/07 17:17:33, elad.alon wrote: > > > On 2017/03/07 16:47:05, minyue-webrtc wrote: > > > > no need for > > > > > > > > EXPECT_EQ(static_cast<bool> > > > > and if > > > > just > > > > > > > > EXPECT_EQ(expected_plr, plr) > > > > > > This is intentional - it yields much clearer error messages (see comment > above > > > the code in question to that effect, btw). Please refer to: > > > > > > https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... > > > I think the example I've given there makes a very compelling case for > keeping > > > things like that. > > > > Sorry. I missed the old comment. Yes, I agree with that. But I think that > might > > be improved in rtc::Optional. For now, how about just adding more message. say > > > > EXPECT_EQ(expected_plr, plr) << expected_plr.value_or(-1.0) << " vs " << > > plr.value_or(-1.0); > > Good idea - I'll implement that in a separate CL. Acknowledged. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:110: if (expected_plr && plr) { On 2017/03/08 19:13:46, elad.alon wrote: > On 2017/03/08 10:24:06, elad.alon wrote: > > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > > On 2017/03/07 17:17:33, elad.alon wrote: > > > > On 2017/03/07 16:47:05, minyue-webrtc wrote: > > > > > no need for > > > > > > > > > > EXPECT_EQ(static_cast<bool> > > > > > and if > > > > > just > > > > > > > > > > EXPECT_EQ(expected_plr, plr) > > > > > > > > This is intentional - it yields much clearer error messages (see comment > > above > > > > the code in question to that effect, btw). Please refer to: > > > > > > > > > > https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... > > > > I think the example I've given there makes a very compelling case for > > keeping > > > > things like that. > > > > > > Sorry. I missed the old comment. Yes, I agree with that. But I think that > > might > > > be improved in rtc::Optional. For now, how about just adding more message. > say > > > > > > EXPECT_EQ(expected_plr, plr) << expected_plr.value_or(-1.0) << " vs " << > > > plr.value_or(-1.0); > > > > Good idea - I'll implement that in a separate CL. > > Sorry, now that I've come back to this, meaning to improve rtc::Optional, I see > that you've suggested .value_or(). IMHO, this would not be an improvement, as > -1.0 is a misleading theoretically-possible value, rather than indicating that > the rtc::Optional is unset. That is to say, true/false are not values that an > rtc::Optional<float> could take on, so an error message which features them make > it immediately clear that the problem is with being set/unset, rather than > somehow actually reaching the value -1.0. So I'd rather keep it as is. I suggested -1.0 because it would be the easiest. When you get an error, you check the code, and you understand it. We can use the display trick to make more accurate error message. Anyways, what I suggested was to use such a trick instead of using more complicated logics. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:174: SendPackets(&tracker, base_, 3, 10); On 2017/03/08 10:24:07, elad.alon wrote: > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > I think SendPackets can just use a default sent interval, because it looks > like > > that it is always 10, and other values does not give more value. > > > > Yes, I see a 0, but you can use explicit addPacket there. > > Earlier CL had a default value, then I changed that based on discussion with > Michael. For Michael, the main selling point for passing value explicitly was a > general preference for that (IIRC). For me, the main selling point is that if we > have a function with *multiple* default values, and their types are not distinct > enough, it's possible to accidentally pass an integer and end up landing it in > the boolean, or vice versa. (Case in point - |send_time_interval_ms| and > |validate_all|.) What I meant was to hard code the interval in SendPackets(), and remove it from the args. Please consider, but may be it is not good. I see other values, like 1ms. But, is 1ms really needed. See comment below. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:248: TEST_P(TransportFeedbackPacketLossTrackerTest, DifferentPacketLengthSanity) { On 2017/03/08 10:24:06, elad.alon wrote: > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > Sounds very specific to packet length, which is not necessarily known to the > > packet loss tracker. > > > > How about DifferentSentIntervals? > > I want to make it clear that the tracker correctly handles different frame > lengths, regardless of DTX, etc. That is, to prove that the trackers doesn't > *need* to know about the frame length. But this might be misleading because the sent time may not increment exactly according to now_ms += frame[i] and people without background knowledge won't know why {20, 60, 120} differs from {40, 70, 90} or anything else. I would like to drop the name of packet length. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:267: TEST_P(TransportFeedbackPacketLossTrackerTest, MaxWindowSize) { On 2017/03/08 10:24:07, elad.alon wrote: > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > I think the test can be made simpler. How about > > > > add a packet -> advance time -> check that old packet is out of window. > > One packet will *not* be out of window in this suggested scenario, by design. We > only move the window when we get new acked packets. Reminder - RTT would > otherwise have to be taken into account, etc., etc... :-) 1. if one packet should not be shifted out by design, should we unittest it? 2. adding 20 packets may still be unnecessary to the test. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:282: AddTransportFeedbackAndValidate(&tracker, base_ + 10, {true}); The fact that we always use Packet Loss Statistics to infer the window content is not stable and hard to understand also. Try consider other means (now in this CL) https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:378: // Moving a window, if if it excludes some old acked messages, can leave if if - > if https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:393: const int64_t moved_oldest_acked = base_ + 2 * max_window_size_ms; base_ is a sequence number, window size is time concept. weird to add them up. I understand that you do 1ms increment, but it is still strange. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:502: TEST_P(TransportFeedbackPacketLossTrackerTest, TimeDifferenceMaximumObserved) { MaxTimeDifferenceCanBeReached https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:531: TEST_P(TransportFeedbackPacketLossTrackerTest, ResetByRepeatedSeqNum) { RepeatedSeqNumResetsWindow https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:550: TEST_P(TransportFeedbackPacketLossTrackerTest, ResetByTooNew) { SendAfterLongSuspensionResetsWindow
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:55: void SendPackets(TransportFeedbackPacketLossTracker* tracker, On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/08 10:24:07, elad.alon wrote: > > On 2017/03/08 09:12:25, minyue-webrtc(OOO_until_Mar19) wrote: > > > why renamed from SendPacketRange, anyways, I like the new name > > > > 1. I am confused - do you favor the change or the name before it? > > 2. I am, naturally, for the new name. I think "Range" is misleading when we > use > > "base and sequence length" rather than "base and last elements". > > Refactoring is generally fine in every CL. But it may make the reviewer think > whether it is related to the main purpose of the CL. So it would be good to add > a comment to say "this is refactoring" when you send out a CL. > > This case is in fact a good refactoring. Go ahead. Acknowledged. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:174: SendPackets(&tracker, base_, 3, 10); On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/08 10:24:07, elad.alon wrote: > > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > > I think SendPackets can just use a default sent interval, because it looks > > like > > > that it is always 10, and other values does not give more value. > > > > > > Yes, I see a 0, but you can use explicit addPacket there. > > > > Earlier CL had a default value, then I changed that based on discussion with > > Michael. For Michael, the main selling point for passing value explicitly was > a > > general preference for that (IIRC). For me, the main selling point is that if > we > > have a function with *multiple* default values, and their types are not > distinct > > enough, it's possible to accidentally pass an integer and end up landing it in > > the boolean, or vice versa. (Case in point - |send_time_interval_ms| and > > |validate_all|.) > > What I meant was to hard code the interval in SendPackets(), and remove it from > the args. Please consider, but may be it is not good. I see other values, like > 1ms. But, is 1ms really needed. See comment below. IMHO, for the 1ms cases, using 1ms simplifies the logic and makes the test better to understand. 1. Examine MaxUnackedPackets, for instance - we don't need to scale (base_ + 0x8000 - 0x2000) by the frame length, and things are therefore easier to follow. 2. In SameSentTime, we can use 0ms as the interval, and this way still use SendPackets(), rather than call OnPacketAdded() three times with the same timestamp, which would deviate from other tests enough to be distracting. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:248: TEST_P(TransportFeedbackPacketLossTrackerTest, DifferentPacketLengthSanity) { On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/08 10:24:06, elad.alon wrote: > > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > > Sounds very specific to packet length, which is not necessarily known to the > > > packet loss tracker. > > > > > > How about DifferentSentIntervals? > > > > I want to make it clear that the tracker correctly handles different frame > > lengths, regardless of DTX, etc. That is, to prove that the trackers doesn't > > *need* to know about the frame length. > > But this might be misleading because the sent time may not increment exactly > according to > > now_ms += frame[i] > > and people without background knowledge won't know why {20, 60, 120} differs > from {40, 70, 90} or anything else. > > I would like to drop the name of packet length. Done. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:267: TEST_P(TransportFeedbackPacketLossTrackerTest, MaxWindowSize) { On 2017/03/14 14:49:28, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/08 10:24:07, elad.alon wrote: > > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > > I think the test can be made simpler. How about > > > > > > add a packet -> advance time -> check that old packet is out of window. > > > > One packet will *not* be out of window in this suggested scenario, by design. > We > > only move the window when we get new acked packets. Reminder - RTT would > > otherwise have to be taken into account, etc., etc... :-) > > 1. if one packet should not be shifted out by design, should we unittest it? > 2. adding 20 packets may still be unnecessary to the test. 1. The tracker is reactive. It doesn't execute any code when time passes, only when events happen - packet transmission or feedback reception - and those events are covered. 2. I don't think making it 15 instead of 20 would matter. Or have I misunderstood? https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:282: AddTransportFeedbackAndValidate(&tracker, base_ + 10, {true}); On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > The fact that we always use Packet Loss Statistics to infer the window content > is not stable and hard to understand also. Try consider other means (now in this > CL) That's a limitation of black-box testing. I can poke a hole and make it white-box, but I got the impression this would not be well accepted by reviewers, who appeared to be unhappy even with Validate(). Do you have any idea in mind? https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:378: // Moving a window, if if it excludes some old acked messages, can leave On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > if if - > if Thanks. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:393: const int64_t moved_oldest_acked = base_ + 2 * max_window_size_ms; On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > base_ is a sequence number, window size is time concept. weird to add them up. > I understand that you do 1ms increment, but it is still strange. The alternative would be to have this code structure exactly, but with "-times-frame-length" each time, which I think would be distracting. If you feel strongly about it, though, I could do that. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:502: TEST_P(TransportFeedbackPacketLossTrackerTest, TimeDifferenceMaximumObserved) { On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > MaxTimeDifferenceCanBeReached "MaxTimeDifferenceCanBeReached" suggests we can hold up to the maximum, but I'm also checking that we properly discard what we were supposed to discard, and still get correct outputs, as I believe "TimeDifferenceMaximumObserved" suggests. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:531: TEST_P(TransportFeedbackPacketLossTrackerTest, ResetByRepeatedSeqNum) { On 2017/03/14 14:49:28, minyue-webrtc(OOO_until_Mar19) wrote: > RepeatedSeqNumResetsWindow Done. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:550: TEST_P(TransportFeedbackPacketLossTrackerTest, ResetByTooNew) { On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > SendAfterLongSuspensionResetsWindow Done.
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:174: SendPackets(&tracker, base_, 3, 10); On 2017/03/14 16:36:52, elad.alon wrote: > On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > > On 2017/03/08 10:24:07, elad.alon wrote: > > > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > > > I think SendPackets can just use a default sent interval, because it looks > > > like > > > > that it is always 10, and other values does not give more value. > > > > > > > > Yes, I see a 0, but you can use explicit addPacket there. > > > > > > Earlier CL had a default value, then I changed that based on discussion with > > > Michael. For Michael, the main selling point for passing value explicitly > was > > a > > > general preference for that (IIRC). For me, the main selling point is that > if > > we > > > have a function with *multiple* default values, and their types are not > > distinct > > > enough, it's possible to accidentally pass an integer and end up landing it > in > > > the boolean, or vice versa. (Case in point - |send_time_interval_ms| and > > > |validate_all|.) > > > > What I meant was to hard code the interval in SendPackets(), and remove it > from > > the args. Please consider, but may be it is not good. I see other values, like > > 1ms. But, is 1ms really needed. See comment below. > > IMHO, for the 1ms cases, using 1ms simplifies the logic and makes the test > better to understand. > 1. Examine MaxUnackedPackets, for instance - we don't need to scale (base_ + > 0x8000 - 0x2000) by the frame length, and things are therefore easier to follow. > 2. In SameSentTime, we can use 0ms as the interval, and this way still use > SendPackets(), rather than call OnPacketAdded() three times with the same > timestamp, which would deviate from other tests enough to be distracting. Acknowledged. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:267: TEST_P(TransportFeedbackPacketLossTrackerTest, MaxWindowSize) { On 2017/03/14 16:36:52, elad.alon wrote: > On 2017/03/14 14:49:28, minyue-webrtc(OOO_until_Mar19) wrote: > > On 2017/03/08 10:24:07, elad.alon wrote: > > > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > > > I think the test can be made simpler. How about > > > > > > > > add a packet -> advance time -> check that old packet is out of window. > > > > > > One packet will *not* be out of window in this suggested scenario, by > design. > > We > > > only move the window when we get new acked packets. Reminder - RTT would > > > otherwise have to be taken into account, etc., etc... :-) > > > > 1. if one packet should not be shifted out by design, should we unittest it? > > 2. adding 20 packets may still be unnecessary to the test. > > 1. The tracker is reactive. It doesn't execute any code when time passes, only > when events happen - packet transmission or feedback reception - and those > events are covered. > 2. I don't think making it 15 instead of 20 would matter. Or have I > misunderstood? I think simpler is better. It feels to me that {false, true, true, false, true, false, false, false, false, true} is an unnecessarily along list. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:282: AddTransportFeedbackAndValidate(&tracker, base_ + 10, {true}); On 2017/03/14 16:36:52, elad.alon wrote: > On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > > The fact that we always use Packet Loss Statistics to infer the window content > > is not stable and hard to understand also. Try consider other means (now in > this > > CL) > > That's a limitation of black-box testing. I can poke a hole and make it > white-box, but I got the impression this would not be well accepted by > reviewers, who appeared to be unhappy even with Validate(). > Do you have any idea in mind? poking a hole isn't a bad idea. Maybe we can add a public getter method to output the window (const reference), and use it only for testing. You may consider this in a separate CL. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:393: const int64_t moved_oldest_acked = base_ + 2 * max_window_size_ms; On 2017/03/14 16:36:52, elad.alon wrote: > On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > > base_ is a sequence number, window size is time concept. weird to add them > up. > > I understand that you do 1ms increment, but it is still strange. > > The alternative would be to have this code structure exactly, but with > "-times-frame-length" each time, which I think would be distracting. If you feel > strongly about it, though, I could do that. two solutions can be good 1. do "- times-frame-length" 2. add a comment that "since time interval is 1ms, diff in base number equal the diff in time", (but this is only true when time unit is millisecond I prefer 1. Then you can use 10ms interval. Then most tests uses 10ms. I then still suggest a hardcoded interval for this test. You may 1. add "constexpr int kDefaultSentIntervalMs = 10" in unnamed namespace 2. remove the arg "send_time_interval_ms" from the method SendPackets() and hardcode kDefaultSentIntervalMs in the method. 3. use kDefaultSentIntervalMs in this test to relate seq num and time 4. change two tests that uses special sent interval Alternatively, you may avoid 2, but put kDefaultSentIntervalMs everywhere (that is more readable) https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:502: TEST_P(TransportFeedbackPacketLossTrackerTest, TimeDifferenceMaximumObserved) { On 2017/03/14 16:36:52, elad.alon wrote: > On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > > MaxTimeDifferenceCanBeReached > > "MaxTimeDifferenceCanBeReached" suggests we can hold up to the maximum, but I'm > also checking that we properly discard what we were supposed to discard, and > still get correct outputs, as I believe "TimeDifferenceMaximumObserved" > suggests. The name is still a bit hard for me to follow. Please update the comment on line 501 https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:520: // Another packet, sent 1ms later, would already be too late. The window will 1ms?
https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:267: TEST_P(TransportFeedbackPacketLossTrackerTest, MaxWindowSize) { On 2017/03/15 10:15:26, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/14 16:36:52, elad.alon wrote: > > On 2017/03/14 14:49:28, minyue-webrtc(OOO_until_Mar19) wrote: > > > On 2017/03/08 10:24:07, elad.alon wrote: > > > > On 2017/03/08 09:12:26, minyue-webrtc(OOO_until_Mar19) wrote: > > > > > I think the test can be made simpler. How about > > > > > > > > > > add a packet -> advance time -> check that old packet is out of window. > > > > > > > > One packet will *not* be out of window in this suggested scenario, by > > design. > > > We > > > > only move the window when we get new acked packets. Reminder - RTT would > > > > otherwise have to be taken into account, etc., etc... :-) > > > > > > 1. if one packet should not be shifted out by design, should we unittest it? > > > 2. adding 20 packets may still be unnecessary to the test. > > > > 1. The tracker is reactive. It doesn't execute any code when time passes, only > > when events happen - packet transmission or feedback reception - and those > > events are covered. > > 2. I don't think making it 15 instead of 20 would matter. Or have I > > misunderstood? > I think simpler is better. It feels to me that {false, true, true, false, true, > false, false, false, false, true} is an unnecessarily along list. > Reduced in size. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:282: AddTransportFeedbackAndValidate(&tracker, base_ + 10, {true}); On 2017/03/15 10:15:26, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/14 16:36:52, elad.alon wrote: > > On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > > > The fact that we always use Packet Loss Statistics to infer the window > content > > > is not stable and hard to understand also. Try consider other means (now in > > this > > > CL) > > > > That's a limitation of black-box testing. I can poke a hole and make it > > white-box, but I got the impression this would not be well accepted by > > reviewers, who appeared to be unhappy even with Validate(). > > Do you have any idea in mind? > > poking a hole isn't a bad idea. Maybe we can add a public getter method to > output the window (const reference), and use it only for testing. You may > consider this in a separate CL. Okay. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:393: const int64_t moved_oldest_acked = base_ + 2 * max_window_size_ms; On 2017/03/15 10:15:26, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/14 16:36:52, elad.alon wrote: > > On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > > > base_ is a sequence number, window size is time concept. weird to add them > > up. > > > I understand that you do 1ms increment, but it is still strange. > > > > The alternative would be to have this code structure exactly, but with > > "-times-frame-length" each time, which I think would be distracting. If you > feel > > strongly about it, though, I could do that. > > two solutions can be good > 1. do "- times-frame-length" > 2. add a comment that "since time interval is 1ms, diff in base number equal the > diff in time", (but this is only true when time unit is millisecond > > I prefer 1. Then you can use 10ms interval. Then most tests uses 10ms. I then > still suggest a hardcoded interval for this test. You may > 1. add "constexpr int kDefaultSentIntervalMs = 10" in unnamed namespace > 2. remove the arg "send_time_interval_ms" from the method SendPackets() and > hardcode kDefaultSentIntervalMs in the method. > 3. use kDefaultSentIntervalMs in this test to relate seq num and time > 4. change two tests that uses special sent interval > > Alternatively, you may avoid 2, but put kDefaultSentIntervalMs everywhere (that > is more readable) The problem with hard-coding the interval is that all of the tests implicitly depend on it when they construct the tracker, but passing the ctor (test-specific-value * kDefaultSentIntervalMs) will: 1. Obfuscate. 2. Create the appearance that the send interval is necessarily constant. I feel that it's clearer to keep it as is, so I'll go with the second of the options you have presented - add a comment. Let's talk offline if this does not satisfy you. (I could also send you a patch which would show what this change entails, which would explain why I believe this to be a step backwards in term of readability.) Actually, there is already a comment to that effect a few lines above - "Note: All messages..." I have extended it to make it a bit clearer. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:502: TEST_P(TransportFeedbackPacketLossTrackerTest, TimeDifferenceMaximumObserved) { On 2017/03/15 10:15:26, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/14 16:36:52, elad.alon wrote: > > On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > > > MaxTimeDifferenceCanBeReached > > > > "MaxTimeDifferenceCanBeReached" suggests we can hold up to the maximum, but > I'm > > also checking that we properly discard what we were supposed to discard, and > > still get correct outputs, as I believe "TimeDifferenceMaximumObserved" > > suggests. > > The name is still a bit hard for me to follow. Please update the comment on line > 501 Done. https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:520: // Another packet, sent 1ms later, would already be too late. The window will On 2017/03/15 10:15:26, minyue-webrtc(OOO_until_Mar19) wrote: > 1ms? Thanks for catching. This appears to be a victim of a previous CR-response. Coverage is better with 1ms instead of 10ms, since otherwise (using 10ms), one could wonder if exceeding the maximum by just a bit - say by 5ms - would have failed the test. I'll revert to the original form of the test, with intervals being 1ms.
lgtm Thanks for the work! A few last comments, and you have the right to decide on them https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:393: const int64_t moved_oldest_acked = base_ + 2 * max_window_size_ms; On 2017/03/15 11:19:44, elad.alon wrote: > On 2017/03/15 10:15:26, minyue-webrtc(OOO_until_Mar19) wrote: > > On 2017/03/14 16:36:52, elad.alon wrote: > > > On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > > > > base_ is a sequence number, window size is time concept. weird to add > them > > > up. > > > > I understand that you do 1ms increment, but it is still strange. > > > > > > The alternative would be to have this code structure exactly, but with > > > "-times-frame-length" each time, which I think would be distracting. If you > > feel > > > strongly about it, though, I could do that. > > > > two solutions can be good > > 1. do "- times-frame-length" > > 2. add a comment that "since time interval is 1ms, diff in base number equal > the > > diff in time", (but this is only true when time unit is millisecond > > > > I prefer 1. Then you can use 10ms interval. Then most tests uses 10ms. I then > > still suggest a hardcoded interval for this test. You may > > 1. add "constexpr int kDefaultSentIntervalMs = 10" in unnamed namespace > > 2. remove the arg "send_time_interval_ms" from the method SendPackets() and > > hardcode kDefaultSentIntervalMs in the method. > > 3. use kDefaultSentIntervalMs in this test to relate seq num and time > > 4. change two tests that uses special sent interval > > > > Alternatively, you may avoid 2, but put kDefaultSentIntervalMs everywhere > (that > > is more readable) > > The problem with hard-coding the interval is that all of the tests implicitly > depend on it when they construct the tracker, but passing the ctor > (test-specific-value * kDefaultSentIntervalMs) will: > 1. Obfuscate. > 2. Create the appearance that the send interval is necessarily constant. Not really, particularly, if you call it default interval. And the fact that the API of the tracker takes arbitrary timestamp cannot hardly be ignored. > > I feel that it's clearer to keep it as is, so I'll go with the second of the > options you have presented - add a comment. Let's talk offline if this does not > satisfy you. (I could also send you a patch which would show what this change > entails, which would explain why I believe this to be a step backwards in term > of readability.) > > Actually, there is already a comment to that effect a few lines above - "Note: > All messages..." I have extended it to make it a bit clearer. OK. but you are sacrificing readability be assuming the readers know that the time unit is millisecond. I let you make final decision anyways. One thing in general: try to use constexpr on numbers to give meanings to them.
On 2017/03/15 13:04:58, minyue-webrtc(OOO_until_Mar19) wrote: > lgtm > > Thanks for the work! A few last comments, and you have the right to decide on > them > > https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... > File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc > (right): > > https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... > webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:393: > const int64_t moved_oldest_acked = base_ + 2 * max_window_size_ms; > On 2017/03/15 11:19:44, elad.alon wrote: > > On 2017/03/15 10:15:26, minyue-webrtc(OOO_until_Mar19) wrote: > > > On 2017/03/14 16:36:52, elad.alon wrote: > > > > On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > > > > > base_ is a sequence number, window size is time concept. weird to add > > them > > > > up. > > > > > I understand that you do 1ms increment, but it is still strange. > > > > > > > > The alternative would be to have this code structure exactly, but with > > > > "-times-frame-length" each time, which I think would be distracting. If > you > > > feel > > > > strongly about it, though, I could do that. > > > > > > two solutions can be good > > > 1. do "- times-frame-length" > > > 2. add a comment that "since time interval is 1ms, diff in base number equal > > the > > > diff in time", (but this is only true when time unit is millisecond > > > > > > I prefer 1. Then you can use 10ms interval. Then most tests uses 10ms. I > then > > > still suggest a hardcoded interval for this test. You may > > > 1. add "constexpr int kDefaultSentIntervalMs = 10" in unnamed namespace > > > 2. remove the arg "send_time_interval_ms" from the method SendPackets() and > > > hardcode kDefaultSentIntervalMs in the method. > > > 3. use kDefaultSentIntervalMs in this test to relate seq num and time > > > 4. change two tests that uses special sent interval > > > > > > Alternatively, you may avoid 2, but put kDefaultSentIntervalMs everywhere > > (that > > > is more readable) > > > > The problem with hard-coding the interval is that all of the tests implicitly > > depend on it when they construct the tracker, but passing the ctor > > (test-specific-value * kDefaultSentIntervalMs) will: > > 1. Obfuscate. > > 2. Create the appearance that the send interval is necessarily constant. > Not really, particularly, if you call it default interval. And the fact that the > API of the tracker takes arbitrary timestamp cannot hardly be ignored. > > > > > I feel that it's clearer to keep it as is, so I'll go with the second of the > > options you have presented - add a comment. Let's talk offline if this does > not > > satisfy you. (I could also send you a patch which would show what this change > > entails, which would explain why I believe this to be a step backwards in term > > of readability.) > > > > Actually, there is already a comment to that effect a few lines above - "Note: > > All messages..." I have extended it to make it a bit clearer. > > OK. but you are sacrificing readability be assuming the readers know that the > time unit is millisecond. > > I let you make final decision anyways. > > One thing in general: try to use constexpr on numbers to give meanings to them. I think I'll land this CL as-is, then send you a separate CL which hard-codes these values. Rationale: 1. Reduce the stack size now, to unblock subsequent CLs which do not really rely on the UTs here. 2. Separate CL gives better focus on whether the changes really improve readability.
On 2017/03/15 13:11:42, elad.alon wrote: > On 2017/03/15 13:04:58, minyue-webrtc(OOO_until_Mar19) wrote: > > lgtm > > > > Thanks for the work! A few last comments, and you have the right to decide on > > them > > > > > https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... > > File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc > > (right): > > > > > https://codereview.webrtc.org/2711473003/diff/140001/webrtc/voice_engine/tran... > > webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:393: > > const int64_t moved_oldest_acked = base_ + 2 * max_window_size_ms; > > On 2017/03/15 11:19:44, elad.alon wrote: > > > On 2017/03/15 10:15:26, minyue-webrtc(OOO_until_Mar19) wrote: > > > > On 2017/03/14 16:36:52, elad.alon wrote: > > > > > On 2017/03/14 14:49:27, minyue-webrtc(OOO_until_Mar19) wrote: > > > > > > base_ is a sequence number, window size is time concept. weird to add > > > them > > > > > up. > > > > > > I understand that you do 1ms increment, but it is still strange. > > > > > > > > > > The alternative would be to have this code structure exactly, but with > > > > > "-times-frame-length" each time, which I think would be distracting. If > > you > > > > feel > > > > > strongly about it, though, I could do that. > > > > > > > > two solutions can be good > > > > 1. do "- times-frame-length" > > > > 2. add a comment that "since time interval is 1ms, diff in base number > equal > > > the > > > > diff in time", (but this is only true when time unit is millisecond > > > > > > > > I prefer 1. Then you can use 10ms interval. Then most tests uses 10ms. I > > then > > > > still suggest a hardcoded interval for this test. You may > > > > 1. add "constexpr int kDefaultSentIntervalMs = 10" in unnamed namespace > > > > 2. remove the arg "send_time_interval_ms" from the method SendPackets() > and > > > > hardcode kDefaultSentIntervalMs in the method. > > > > 3. use kDefaultSentIntervalMs in this test to relate seq num and time > > > > 4. change two tests that uses special sent interval > > > > > > > > Alternatively, you may avoid 2, but put kDefaultSentIntervalMs everywhere > > > (that > > > > is more readable) > > > > > > The problem with hard-coding the interval is that all of the tests > implicitly > > > depend on it when they construct the tracker, but passing the ctor > > > (test-specific-value * kDefaultSentIntervalMs) will: > > > 1. Obfuscate. > > > 2. Create the appearance that the send interval is necessarily constant. > > Not really, particularly, if you call it default interval. And the fact that > the > > API of the tracker takes arbitrary timestamp cannot hardly be ignored. > > > > > > > > I feel that it's clearer to keep it as is, so I'll go with the second of the > > > options you have presented - add a comment. Let's talk offline if this does > > not > > > satisfy you. (I could also send you a patch which would show what this > change > > > entails, which would explain why I believe this to be a step backwards in > term > > > of readability.) > > > > > > Actually, there is already a comment to that effect a few lines above - > "Note: > > > All messages..." I have extended it to make it a bit clearer. > > > > OK. but you are sacrificing readability be assuming the readers know that the > > time unit is millisecond. > > > > I let you make final decision anyways. > > > > One thing in general: try to use constexpr on numbers to give meanings to > them. > > I think I'll land this CL as-is, then send you a separate CL which hard-codes > these values. Rationale: > 1. Reduce the stack size now, to unblock subsequent CLs which do not really rely > on the UTs here. > 2. Separate CL gives better focus on whether the changes really improve > readability. s/hard-codes/parameterizes
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelt@webrtc.org, pbos@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2711473003/#ps280001 (title: "CR response")
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 elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelt@webrtc.org, minyue@webrtc.org, solenberg@webrtc.org, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2711473003/#ps300001 (title: "Revert emplace_hint; not all compilers support.")
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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/23746)
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelt@webrtc.org, minyue@webrtc.org, solenberg@webrtc.org, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2711473003/#ps320001 (title: "More compiler nits.")
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": 320001, "attempt_start_ts": 1489586811955240,
"parent_rev": "be35a008ef39bc52a1ac30e763057d0ab5df824a", "commit_rev":
"3f9b12d0564fb8e5c640b1cf114a8b763b1ca49d"}
Message was sent while issue was closed.
Description was changed from ========== R/PLR calculation - time-based window BUG=webrtc:7058 ========== to ========== R/PLR calculation - time-based window BUG=webrtc:7058 Review-Url: https://codereview.webrtc.org/2711473003 Cr-Commit-Position: refs/heads/master@{#17251} Committed: https://chromium.googlesource.com/external/webrtc/+/3f9b12d0564fb8e5c640b1cf1... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/external/webrtc/+/3f9b12d0564fb8e5c640b1cf1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
