Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632203002:
Packet Loss Tracker - Stream Separation (Closed)
|
Created:
3 years, 11 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 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPacket Loss Tracker - Stream Separation
1. Packet reception statistics (PLR and RPLR) calculated on each stream separately.
2. Algorithm changed from considering separate quadrants of the sequence-number-ring to simply looking at the oldest in-window SENT packet.
BUG=webrtc:7058
Review-Url: https://codereview.webrtc.org/2632203002
Cr-Commit-Position: refs/heads/master@{#17018}
Committed: https://chromium.googlesource.com/external/webrtc/+/7af935758037dc483581ea701624cdb1a4dc77af
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Fuzzer (TODO) #Patch Set 4 : Fix Validate() problem. Fuzzer not done. #Patch Set 5 : Fuzzer working #Patch Set 6 : nit #Patch Set 7 : Updated UT #Patch Set 8 : Some refactoring #Patch Set 9 : Rebase #Patch Set 10 : Rebased #Patch Set 11 : . #Patch Set 12 : . #Patch Set 13 : nit #Patch Set 14 : s/Sent/Unacked and some more clarifying comments. #Patch Set 15 : . #Patch Set 16 : Rebased #Patch Set 17 : Rebased #
Total comments: 3
Patch Set 18 : . #
Total comments: 10
Patch Set 19 : . #Patch Set 20 : hint #
Total comments: 62
Patch Set 21 : CR response #
Total comments: 5
Patch Set 22 : . #
Total comments: 11
Patch Set 23 : CR response #Patch Set 24 : . #Patch Set 25 : UT fix #Patch Set 26 : Add AllLost UT (+nits). #Patch Set 27 : Remove unused feedback. #Patch Set 28 : nit: Fix some comments in the fuzzer. #Patch Set 29 : . #
Total comments: 27
Patch Set 30 : Stricter compiler on server; appeased? #Patch Set 31 : . #
Dependent Patchsets: Messages
Total messages: 50 (14 generated)
Description was changed from ========== Packet Loss Tracker - Stream Separation 1. Packet reception statistics (PLR and RPLR) calculated on each stream separately. 2. Algorithm changed from considering separate quadrants of the sequence-number-ring to simply looking at the oldest in-window SENT packet. ========== to ========== Packet Loss Tracker - Stream Separation 1. Packet reception statistics (PLR and RPLR) calculated on each stream separately. 2. Algorithm changed from considering separate quadrants of the sequence-number-ring to simply looking at the oldest in-window SENT packet. ==========
elad.alon@webrtc.org changed reviewers: + michaelt@webrtc.org, minyue@webrtc.org, sprang@webrtc.org, stefan@webrtc.org
Description was changed from ========== Packet Loss Tracker - Stream Separation 1. Packet reception statistics (PLR and RPLR) calculated on each stream separately. 2. Algorithm changed from considering separate quadrants of the sequence-number-ring to simply looking at the oldest in-window SENT packet. ========== to ========== Packet Loss Tracker - Stream Separation 1. Packet reception statistics (PLR and RPLR) calculated on each stream separately. 2. Algorithm changed from considering separate quadrants of the sequence-number-ring to simply looking at the oldest in-window SENT packet. BUG=webrtc:7058 ==========
Ready for review, btw.
https://codereview.webrtc.org/2632203002/diff/320001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/320001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:65: uint16_t TransportFeedbackPacketLossTracker::NewestSequenceNumber() const { nit: try keeping methods in the same order as in the .h
https://codereview.webrtc.org/2632203002/diff/320001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/320001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:65: uint16_t TransportFeedbackPacketLossTracker::NewestSequenceNumber() const { On 2017/02/07 15:52:44, the sun wrote: > nit: try keeping methods in the same order as in the .h I've tried to (NewestSequenceNumber after ReferenceSequenceNumber, OnPacketAdded before OnReceivedTransportFeedback). The problem is that the baseline is not ordered - Reset() is right after the ctor, probably because it's related. Shall I create an additional CL to reorder the functions?
https://codereview.webrtc.org/2632203002/diff/320001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (left): https://codereview.webrtc.org/2632203002/diff/320001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:56: plr_state_.Reset(); @stefan + @sprang: The copy-assignment operator is implicitly deleted. IMHO, would be simplest to just keep this Reset(). Wdyt?
Some more general questions. The class seams already quite big an complicated. So i would love to separate the logic in multiple classes. We may could for example separate the ring buffer ? I see some problems with the idea to make the window size packet based. You may can adapt for the different FL, but what is about DTX or what will you do if we mix audio and video streams. I would vote for a time based window size. https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:74: if (packet_status_window_.find(seq_num) != packet_status_window_.end()) { The "else if" seams to be obsolete. Would go for: if (packet_status_window_.find(seq_num) != packet_status_window_.end() || !packet_status_window_.empty() && ForwardDiff(seq_num, NewestSequenceNumber()) <= kSeqNumHalf) { Reset(); } https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:87: const auto& ret = packet_status_window_.insert( 1. "packet_status" would make more sene to me for a name than "ret" 2. Using a hint here should make this more efficient. https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:89: RTC_DCHECK(ret.second); Seams obsolete since you checked already if the seq_num si in the map. https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:165: UpdateMetrics(ref_packet_status_, false); Why do we UpdateMetrics before we remove the packet ? https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:200: default: The default case seams useless to me, since the compiler will warn us anyway if we did not used all cases.
Refactoring might be good. We're working on a time-based solution (which would help with adaptable frame-lengths). I think it would be best to refactor after we've decided what we'll do there, to avoid duplicated effort. https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:74: if (packet_status_window_.find(seq_num) != packet_status_window_.end()) { On 2017/02/09 10:52:02, michaelt wrote: > The "else if" seams to be obsolete. Would go for: > > if (packet_status_window_.find(seq_num) != packet_status_window_.end() || > !packet_status_window_.empty() && > ForwardDiff(seq_num, NewestSequenceNumber()) <= kSeqNumHalf) { > Reset(); > } I think it's easier to read like this, but if the consensus ends up being that it's better to combine the two cases, because they have the same behavior, I'll do so. What do the rest of you think? https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:89: RTC_DCHECK(ret.second); On 2017/02/09 10:52:05, michaelt wrote: > Seams obsolete since you checked already if the seq_num si in the map. I think out-of-memory would be the only way for this to happen. A quick search through the code-base seems to indicate that we don't usually D/CHECK that insertion was successful, so I'll indeed remove this. https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:165: UpdateMetrics(ref_packet_status_, false); On 2017/02/09 10:52:04, michaelt wrote: > Why do we UpdateMetrics before we remove the packet ? The order is: 1. Update the statistics. This packet might have contributed to R/PLR; now that it's being removed, this contribution needs to be forgotten. 2. Remove the packet. Doing it the other way around would mean having to remember the removed packet's properties in a temporary variable. https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:200: default: On 2017/02/09 10:52:03, michaelt wrote: > The default case seams useless to me, since the compiler will warn us anyway if > we did not used all cases. It protects against illegal enum values, such as 3, which I consider good-practice. But I guess the coding convention is king. Minyue, what is the standard here in Google for such cases?
https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/340001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:87: const auto& ret = packet_status_window_.insert( On 2017/02/09 10:52:02, michaelt wrote: > 1. "packet_status" would make more sene to me for a name than "ret" > 2. Using a hint here should make this more efficient. Good point about the hint.
Good work, and please see my comments. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:51: constexpr int64_t kBaseTimeUs = 1234; // Irrelevant to this test. the old place is better. Generally, make it as close to actual use as possible. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:67: while (remaining_packets > 0) { if there any functional changes made in this loop? The old style isn't bad. once we have multiple way to to exit the loop, it is good to add exit(s) (in this case return) inside. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:69: if (!ReadData<uint8_t>(&status_byte)) { nit: remove {} https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:75: if (received) { nit: remove {} https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:101: size_t* size_; size_ -> remaining_bytes_ https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:157: bool FuzzPacketTransmissionBurst( And I think this is not "transmission", which may involve network, it is "send" So I suggest "FuzzSendingPackets" https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:157: bool FuzzPacketTransmissionBurst( I see the "burst" here means burst send instead of burst loss. It may be misleading because people may not think of burst send when seeing "burst." You may drop "burst" as a word. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:170: packet_transmission_burst_len = 0xffff + 1; a little bit too ad hoc. can we packet_transmission_burst_len + 1? since we don't need zero (reading zeros is very inefficient, imagine a file of all zeros.) https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:182: tracker->OnPacketAdded(seq_num); try moving 182,183 in to loop. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:201: bool FuzzTransportFeedbackBurst( try not calling this Bursts either. FuzzTransportFeedbacks https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:241: may_continue = FuzzTransportFeedbackBurst(tracker, &data, &size); it looks like send and feedback are independent. What chance would they overlap? https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:70: void TransportFeedbackPacketLossTracker::OnPacketAdded(uint16_t seq_num) { The name should be OnSentPacket or something like that IMO https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:71: // The only way for these two to is when the stream lies dormant for long .. to ?? is ... and looks like that this comment should be placed inside if https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:77: ForwardDiff(seq_num, NewestSequenceNumber()) <= kSeqNumHalf) { how can this happen? https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:129: PacketStatusIterator it, if we make "it" into seq_num instead, we may avoid "PacketStatusIterator" Then this function can do 1. find(seq_num) 2. if (new is any of the acks && was not found) { return } 3. what we have here already https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:55: // that reports it as received/lost, we update the state and nit: line break after "metrics" https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:71: void RecordPacketStatus(PacketStatusIterator it, Is it really necessary to define PacketStatusIterator https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (left): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:636: // Feedbacks spanning multiple quadrant are treated correctly (Q1-Q2). why the tests below here also become obsolete? https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:34: void SimulatePacketTransmission(TransportFeedbackPacketLossTracker* tracker, "Simulate" may not be needed since the test itself is simulate. "Transmission" -> "Send" since the former sounds like a network thing So generally, I would suggest "SendPacket" https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:65: SimulatePacketTransmission(tracker, first_seq_num + i, validate_all); can we use SimulatePacketTransmission ver.2 to avoid the need of SimulatePacketTransmission ver.1
https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:51: constexpr int64_t kBaseTimeUs = 1234; // Irrelevant to this test. On 2017/02/14 16:46:59, minyue-webrtc wrote: > the old place is better. Generally, make it as close to actual use as possible. Done. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:67: while (remaining_packets > 0) { On 2017/02/14 16:46:58, minyue-webrtc wrote: > if there any functional changes made in this loop? > > The old style isn't bad. once we have multiple way to to exit the loop, it is > good to add exit(s) (in this case return) inside. The previous code was no bad, of course! It's just that circumstances have changed. * Previously, you could only terminate when you were done - regardless of why you were done - and then you'd immediately leave the function altogether. * Now it's possible to either have "early termination" (fuzzed source ran dry) or "proper" termination (fuzzed the right amount of packets). The former is exits the function, the latter only the loop (followed by more code). So to make it easier to read, I state the break condition in the loop condition, where one would expect to find it, and only leave the early termination explicit. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:101: size_t* size_; On 2017/02/14 16:46:59, minyue-webrtc wrote: > size_ -> remaining_bytes_ FuzzOneInput() takes on a parameter called "size", then this value is propagated throughout the fuzzer. IMHO, we either change everywhere or nowhere. Given that other fuzzers use the name "size", I vote for keeping it. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:157: bool FuzzPacketTransmissionBurst( On 2017/02/14 16:46:59, minyue-webrtc wrote: > I see the "burst" here means burst send instead of burst loss. It may be > misleading because people may not think of burst send when seeing "burst." > > You may drop "burst" as a word. Changed to FuzzPacketBlock, to keep it consistent with FuzzTransportFeedbackBlock, where I don't want the word "receiving" to appear. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:170: packet_transmission_burst_len = 0xffff + 1; On 2017/02/14 16:46:59, minyue-webrtc wrote: > a little bit too ad hoc. > > can we packet_transmission_burst_len + 1? since we don't need zero (reading > zeros is very inefficient, imagine a file of all zeros.) The problem is that 0 is a valid case which should be checked. Otherwise, we'd never be running the fuzzer on a window that has no unacked messages. (If you've got an idea how to do it more nicely, I'll be happy to implement it.) https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:182: tracker->OnPacketAdded(seq_num); On 2017/02/14 16:46:58, minyue-webrtc wrote: > try moving 182,183 in to loop. I think it's clearer like this - first packet gets special treatment (could start anywhere, with uniform distribution), then subsequent ones are a delta of the first. I'll modify the loop slightly (make it a for loop that begins at 1) to make it more apparent that it excludes the first element, though. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:201: bool FuzzTransportFeedbackBurst( On 2017/02/14 16:46:58, minyue-webrtc wrote: > try not calling this Bursts either. > > FuzzTransportFeedbacks Done (FuzzTransportFeedbackBlock). https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:241: may_continue = FuzzTransportFeedbackBurst(tracker, &data, &size); On 2017/02/14 16:46:59, minyue-webrtc wrote: > it looks like send and feedback are independent. What chance would they overlap? When running the fuzzer, we do get them to overlap; I've checked. I can check how often, if you'd like, but for now I'll assume that's enough. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:70: void TransportFeedbackPacketLossTracker::OnPacketAdded(uint16_t seq_num) { On 2017/02/14 16:46:59, minyue-webrtc wrote: > The name should be OnSentPacket or something like that IMO This is triggered from the pre-existing TransportFeedbackAdapter::AddPacket (see subsequent CL), which is why I prefer this name. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:71: // The only way for these two to is when the stream lies dormant for long On 2017/02/14 16:46:59, minyue-webrtc wrote: > .. to ?? is ... > > and looks like that this comment should be placed inside if Missing "happen"; thanks for catching. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:77: ForwardDiff(seq_num, NewestSequenceNumber()) <= kSeqNumHalf) { On 2017/02/14 16:46:59, minyue-webrtc wrote: > how can this happen? DTX, for instance. Or anything else that might cause the stream to not produce packets for a long time, while other streams do. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:129: PacketStatusIterator it, On 2017/02/14 16:46:59, minyue-webrtc wrote: > if we make "it" into seq_num instead, we may avoid "PacketStatusIterator" > > Then this function can do > > 1. find(seq_num) > 2. if (new is any of the acks && was not found) { return } > 3. what we have here already > I feel that calling RecordPacketStatus() when it might have no effect is confusing. I think it's clearer like this. Wdyt? https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:55: // that reports it as received/lost, we update the state and On 2017/02/14 16:46:59, minyue-webrtc wrote: > nit: line break after "metrics" Done. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:71: void RecordPacketStatus(PacketStatusIterator it, On 2017/02/14 16:46:59, minyue-webrtc wrote: > Is it really necessary to define PacketStatusIterator We have ConstPacketStatusIterator (earlier named "PacketStatusIterator"). Imho, we either get rid of both, or of neither. Wdys? https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:34: void SimulatePacketTransmission(TransportFeedbackPacketLossTracker* tracker, On 2017/02/14 16:46:59, minyue-webrtc wrote: > "Simulate" may not be needed since the test itself is simulate. > > "Transmission" -> "Send" since the former sounds like a network thing > > So generally, I would suggest "SendPacket" Done. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:65: SimulatePacketTransmission(tracker, first_seq_num + i, validate_all); On 2017/02/14 16:46:59, minyue-webrtc wrote: > can we use SimulatePacketTransmission ver.2 to avoid the need of > SimulatePacketTransmission ver.1 Done.
https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:101: size_t* size_; On 2017/02/15 16:47:35, elad.alon wrote: > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > size_ -> remaining_bytes_ > > FuzzOneInput() takes on a parameter called "size", then this value is propagated > throughout the fuzzer. IMHO, we either change everywhere or nowhere. Given that > other fuzzers use the name "size", I vote for keeping it. Acknowledged. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:170: packet_transmission_burst_len = 0xffff + 1; On 2017/02/15 16:47:35, elad.alon wrote: > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > a little bit too ad hoc. > > > > can we packet_transmission_burst_len + 1? since we don't need zero (reading > > zeros is very inefficient, imagine a file of all zeros.) > > The problem is that 0 is a valid case which should be checked. Otherwise, we'd > never be running the fuzzer on a window that has no unacked messages. > (If you've got an idea how to do it more nicely, I'll be happy to implement it.) I think 0 case should be included in the unittest, or DCHECK, it is not the main purpose of the fuzzer. Or do you see an additional value? If the fuzzer data contains a lot of zeros, allowing zero block length will be inefficient. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:182: tracker->OnPacketAdded(seq_num); On 2017/02/15 16:47:36, elad.alon wrote: > On 2017/02/14 16:46:58, minyue-webrtc wrote: > > try moving 182,183 in to loop. > > I think it's clearer like this - first packet gets special treatment (could > start anywhere, with uniform distribution), then subsequent ones are a delta of > the first. I'll modify the loop slightly (make it a for loop that begins at 1) > to make it more apparent that it excludes the first element, though. Acknowledged. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:241: may_continue = FuzzTransportFeedbackBurst(tracker, &data, &size); On 2017/02/15 16:47:35, elad.alon wrote: > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > it looks like send and feedback are independent. What chance would they > overlap? > > When running the fuzzer, we do get them to overlap; I've checked. I can check > how often, if you'd like, but for now I'll assume that's enough. IMO, making the sending and feedback totally independent like this makes the test spending most of time on "feedback does not hit any previously sent packet", can we do something like maintaining a set of "10 last sent sequence numbers" and pick one of them + a chance of none-of-them when sending feedback. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:70: void TransportFeedbackPacketLossTracker::OnPacketAdded(uint16_t seq_num) { On 2017/02/15 16:47:36, elad.alon wrote: > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > The name should be OnSentPacket or something like that IMO > > This is triggered from the pre-existing TransportFeedbackAdapter::AddPacket (see > subsequent CL), which is why I prefer this name. Should it hook to OnSentPacket instead: see https://cs.chromium.org/chromium/src/third_party/webrtc/modules/congestion_co... https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:129: PacketStatusIterator it, On 2017/02/15 16:47:36, elad.alon wrote: > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > if we make "it" into seq_num instead, we may avoid "PacketStatusIterator" > > > > Then this function can do > > > > 1. find(seq_num) > > 2. if (new is any of the acks && was not found) { return } > > 3. what we have here already > > > > I feel that calling RecordPacketStatus() when it might have no effect is > confusing. I think it's clearer like this. Wdyt? I feel typedef a non-const iterator just for this function is not necessary. I'd like avoiding iterator, since we cannot guarantee that iterator is always valid. If you ask me about ConstPacketStatusIterator, yes, it is good to avoid it too, but I did not find a way that is as efficient. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:71: void RecordPacketStatus(PacketStatusIterator it, On 2017/02/15 16:47:36, elad.alon wrote: > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > Is it really necessary to define PacketStatusIterator > > We have ConstPacketStatusIterator (earlier named "PacketStatusIterator"). Imho, > we either get rid of both, or of neither. Wdys? I like getting rid of both. But I am not sure if we can still achieve the same efficiency. Since ConstPacketStatusIterator was used to avoid duplicating "find". I found that PacketStatusIterator can be avoid without introducing more find(), and const iterator feels safer because you know the function cannot change the window. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (left): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:636: // Feedbacks spanning multiple quadrant are treated correctly (Q1-Q2). On 2017/02/14 16:46:59, minyue-webrtc wrote: > why the tests below here also become obsolete? missed this comment? https://codereview.webrtc.org/2632203002/diff/400001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2632203002/diff/400001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:156: bool FuzzPacketBlock( I do not quite like the name. Unlike TransportFeedback, we do not fuzz any "Packet", we fuzz packet sending event.
https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:241: may_continue = FuzzTransportFeedbackBurst(tracker, &data, &size); On 2017/02/16 09:51:44, minyue-webrtc wrote: > On 2017/02/15 16:47:35, elad.alon wrote: > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > it looks like send and feedback are independent. What chance would they > > overlap? > > > > When running the fuzzer, we do get them to overlap; I've checked. I can check > > how often, if you'd like, but for now I'll assume that's enough. > > IMO, making the sending and feedback totally independent like this makes the > test spending most of time on "feedback does not hit any previously sent > packet", can we do something like > > maintaining a set of "10 last sent sequence numbers" and pick one of them + a > chance of none-of-them when sending feedback. I realized that I was wrong in saying that "feedback, in most time, does not hit any previously sent packet". Hitting can happen because there are a number (up to max_window_size) of sent packets sparsely distributed in the buffer, and the feedback is a continuum of packets, where the number of packets can be large. In that sense, it is fine to let the two be independent. https://codereview.webrtc.org/2632203002/diff/400001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2632203002/diff/400001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:136: // * First seed in range [241 : 255] (~5% chance) -> delta in [1 : 2^16] as discussed offline, the outcome of a large delta can be achieved anyway since we loop Send - Feedback several times.
https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:170: packet_transmission_burst_len = 0xffff + 1; On 2017/02/16 09:51:44, minyue-webrtc wrote: > On 2017/02/15 16:47:35, elad.alon wrote: > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > a little bit too ad hoc. > > > > > > can we packet_transmission_burst_len + 1? since we don't need zero (reading > > > zeros is very inefficient, imagine a file of all zeros.) > > > > The problem is that 0 is a valid case which should be checked. Otherwise, we'd > > never be running the fuzzer on a window that has no unacked messages. > > (If you've got an idea how to do it more nicely, I'll be happy to implement > it.) > > I think 0 case should be included in the unittest, or DCHECK, it is not the main > purpose of the fuzzer. Or do you see an additional value? > > If the fuzzer data contains a lot of zeros, allowing zero block length will be > inefficient. Okay, I'll modify. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:70: void TransportFeedbackPacketLossTracker::OnPacketAdded(uint16_t seq_num) { On 2017/02/16 09:51:44, minyue-webrtc wrote: > On 2017/02/15 16:47:36, elad.alon wrote: > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > The name should be OnSentPacket or something like that IMO > > > > This is triggered from the pre-existing TransportFeedbackAdapter::AddPacket > (see > > subsequent CL), which is why I prefer this name. > > Should it hook to OnSentPacket instead: see > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/congestion_co... That one seems to be called after the packet is sent, which creates a race with the feedback from the other side. It's a race we're very unlikely to lose, but it's still a race. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:129: PacketStatusIterator it, On 2017/02/16 09:51:44, minyue-webrtc wrote: > On 2017/02/15 16:47:36, elad.alon wrote: > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > if we make "it" into seq_num instead, we may avoid "PacketStatusIterator" > > > > > > Then this function can do > > > > > > 1. find(seq_num) > > > 2. if (new is any of the acks && was not found) { return } > > > 3. what we have here already > > > > > > > I feel that calling RecordPacketStatus() when it might have no effect is > > confusing. I think it's clearer like this. Wdyt? > > I feel typedef a non-const iterator just for this function is not necessary. I'd > like avoiding iterator, since we cannot guarantee that iterator is always valid. > > If you ask me about ConstPacketStatusIterator, yes, it is good to avoid it too, > but I did not find a way that is as efficient. I feel like RecordPacketStatus() makes the code clearer, so I've kept it. I did get rid of the typedef for the non-const iterator. I'll keep the ConstPacketStatusIterator's new name, because I feel that it's clearer. But frankly, it only saves a couple of chars, so I'd like to get rid of it completely, too. Wdyt? https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:71: void RecordPacketStatus(PacketStatusIterator it, On 2017/02/16 09:51:44, minyue-webrtc wrote: > On 2017/02/15 16:47:36, elad.alon wrote: > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > Is it really necessary to define PacketStatusIterator > > > > We have ConstPacketStatusIterator (earlier named "PacketStatusIterator"). > Imho, > > we either get rid of both, or of neither. Wdys? > > I like getting rid of both. But I am not sure if we can still achieve the same > efficiency. Since ConstPacketStatusIterator was used to avoid duplicating > "find". > > I found that PacketStatusIterator can be avoid without introducing more find(), > and const iterator feels safer because you know the function cannot change the > window. Answered in .cc file and offline. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (left): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:636: // Feedbacks spanning multiple quadrant are treated correctly (Q1-Q2). On 2017/02/16 09:51:44, minyue-webrtc wrote: > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > why the tests below here also become obsolete? > > missed this comment? Yes, I'd missed the comment; thanks for pointing it out again. These tests are no longer relevant. * Before, we had no notion of unacked message in the window. When a new feedback arrived, it could trigger moving the window in a way that would get old messages forgotten (independently of the window being full or not). * Now, we have the notion of unacked windows in the packet, and it's the addition of new packets that could get old ones discarded. That being said, yes, I see the sense in adding a new test where we get 0x8000 or more in between unacked messages, and therefore reset the window. https://codereview.webrtc.org/2632203002/diff/400001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2632203002/diff/400001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:136: // * First seed in range [241 : 255] (~5% chance) -> delta in [1 : 2^16] On 2017/02/16 13:19:22, minyue-webrtc wrote: > as discussed offline, the outcome of a large delta can be achieved anyway since > we loop Send - Feedback several times. Done. https://codereview.webrtc.org/2632203002/diff/400001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:156: bool FuzzPacketBlock( On 2017/02/16 09:51:44, minyue-webrtc wrote: > I do not quite like the name. Unlike TransportFeedback, we do not fuzz any > "Packet", we fuzz packet sending event. FuzzPacketSendBlock, then?
https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:70: void TransportFeedbackPacketLossTracker::OnPacketAdded(uint16_t seq_num) { On 2017/02/16 15:51:40, elad.alon wrote: > On 2017/02/16 09:51:44, minyue-webrtc wrote: > > On 2017/02/15 16:47:36, elad.alon wrote: > > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > > The name should be OnSentPacket or something like that IMO > > > > > > This is triggered from the pre-existing TransportFeedbackAdapter::AddPacket > > (see > > > subsequent CL), which is why I prefer this name. > > > > Should it hook to OnSentPacket instead: see > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/congestion_co... > > That one seems to be called after the packet is sent, which creates a race with > the feedback from the other side. It's a race we're very unlikely to lose, but > it's still a race. I am not seeing this as a problem. Please refer to my corresponding comment in the follow-up cl. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:129: PacketStatusIterator it, On 2017/02/16 15:51:40, elad.alon wrote: > On 2017/02/16 09:51:44, minyue-webrtc wrote: > > On 2017/02/15 16:47:36, elad.alon wrote: > > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > > if we make "it" into seq_num instead, we may avoid "PacketStatusIterator" > > > > > > > > Then this function can do > > > > > > > > 1. find(seq_num) > > > > 2. if (new is any of the acks && was not found) { return } > > > > 3. what we have here already > > > > > > > > > > I feel that calling RecordPacketStatus() when it might have no effect is > > > confusing. I think it's clearer like this. Wdyt? > > > > I feel typedef a non-const iterator just for this function is not necessary. > I'd > > like avoiding iterator, since we cannot guarantee that iterator is always > valid. > > > > If you ask me about ConstPacketStatusIterator, yes, it is good to avoid it > too, > > but I did not find a way that is as efficient. > > I feel like RecordPacketStatus() makes the code clearer, so I've kept it. I did > get rid of the typedef for the non-const iterator. I'll keep the > ConstPacketStatusIterator's new name, because I feel that it's clearer. But > frankly, it only saves a couple of chars, so I'd like to get rid of it > completely, too. Wdyt? ConstPacketStatusIterator was introduced to avoid duplication, it does appear a lot more often. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:130: PacketStatus new_packet_status) { I see another issue. RecordPacketStatus sounds more generic than its name. Because it is called only for feedback handling but it takes PacketStatus as input, which can be UNACKED. Therefore one may think of either 1) change the scope by making it RecordFeedback(PacketStatusIterator it, bool received) 2) move it entirely to OnReceivedTransportFeedback() if it will not make OnReceivedTransportFeedback() too lengthy https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (left): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:636: // Feedbacks spanning multiple quadrant are treated correctly (Q1-Q2). On 2017/02/16 15:51:40, elad.alon wrote: > On 2017/02/16 09:51:44, minyue-webrtc wrote: > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > why the tests below here also become obsolete? > > > > missed this comment? > > Yes, I'd missed the comment; thanks for pointing it out again. > These tests are no longer relevant. > * Before, we had no notion of unacked message in the window. When a new feedback > arrived, it could trigger moving the window in a way that would get old messages > forgotten (independently of the window being full or not). > * Now, we have the notion of unacked windows in the packet, and it's the > addition of new packets that could get old ones discarded. That being said, yes, > I see the sense in adding a new test where we get 0x8000 or more in between > unacked messages, and therefore reset the window. Ok. then, as you said, the removing of old packets for OnPacketAdded (which I think it still be OnSentPacket) should be tested. https://codereview.webrtc.org/2632203002/diff/400001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2632203002/diff/400001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:156: bool FuzzPacketBlock( On 2017/02/16 15:51:40, elad.alon wrote: > On 2017/02/16 09:51:44, minyue-webrtc wrote: > > I do not quite like the name. Unlike TransportFeedback, we do not fuzz any > > "Packet", we fuzz packet sending event. > > FuzzPacketSendBlock, then? that sounds good, and it is coupled to FuzzTransportFeedbackBlock https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:588: // but does not reset it. it looks like _2 include _1, since if a packet that is less than 0x8000 from the newest does not trigger a reset, why should 0x8000 from baseline trigger a reset. https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:614: // Expected window contents: [] -> [1-GAP-10101]. This test may be made less complicated, how about [10101] -> base + 5 + 0x8000 -> window moved. (like the previous one, which I think can be removed, though) If you find the test not covering enough cases, try divide it into small tests, each of which is to one point. https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:632: // Adding more data proves the window was not reset; it still had one proves -> to prove https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:642: TEST(TransportFeedbackPacketLossTrackerTest, WindowResetByPacketsTooNew) { TooNew may not be good. how about WindowResetAfterLongNoSend?
https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:70: void TransportFeedbackPacketLossTracker::OnPacketAdded(uint16_t seq_num) { On 2017/02/17 11:09:14, minyue-webrtc wrote: > On 2017/02/16 15:51:40, elad.alon wrote: > > On 2017/02/16 09:51:44, minyue-webrtc wrote: > > > On 2017/02/15 16:47:36, elad.alon wrote: > > > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > > > The name should be OnSentPacket or something like that IMO > > > > > > > > This is triggered from the pre-existing > TransportFeedbackAdapter::AddPacket > > > (see > > > > subsequent CL), which is why I prefer this name. > > > > > > Should it hook to OnSentPacket instead: see > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/congestion_co... > > > > That one seems to be called after the packet is sent, which creates a race > with > > the feedback from the other side. It's a race we're very unlikely to lose, but > > it's still a race. > > I am not seeing this as a problem. Please refer to my corresponding comment in > the follow-up cl. (Replied there.) https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:129: PacketStatusIterator it, On 2017/02/17 11:09:14, minyue-webrtc wrote: > On 2017/02/16 15:51:40, elad.alon wrote: > > On 2017/02/16 09:51:44, minyue-webrtc wrote: > > > On 2017/02/15 16:47:36, elad.alon wrote: > > > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > > > if we make "it" into seq_num instead, we may avoid > "PacketStatusIterator" > > > > > > > > > > Then this function can do > > > > > > > > > > 1. find(seq_num) > > > > > 2. if (new is any of the acks && was not found) { return } > > > > > 3. what we have here already > > > > > > > > > > > > > I feel that calling RecordPacketStatus() when it might have no effect is > > > > confusing. I think it's clearer like this. Wdyt? > > > > > > I feel typedef a non-const iterator just for this function is not necessary. > > I'd > > > like avoiding iterator, since we cannot guarantee that iterator is always > > valid. > > > > > > If you ask me about ConstPacketStatusIterator, yes, it is good to avoid it > > too, > > > but I did not find a way that is as efficient. > > > > I feel like RecordPacketStatus() makes the code clearer, so I've kept it. I > did > > get rid of the typedef for the non-const iterator. I'll keep the > > ConstPacketStatusIterator's new name, because I feel that it's clearer. But > > frankly, it only saves a couple of chars, so I'd like to get rid of it > > completely, too. Wdyt? > > ConstPacketStatusIterator was introduced to avoid duplication, it does appear a > lot more often. Do you want the name to be reverted to PacketStatusIterator? https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:130: PacketStatus new_packet_status) { On 2017/02/17 11:09:14, minyue-webrtc wrote: > I see another issue. RecordPacketStatus sounds more generic than its name. > Because it is called only for feedback handling but it takes PacketStatus as > input, which can be UNACKED. Therefore one may think of either > 1) change the scope by making it > RecordFeedback(PacketStatusIterator it, bool received) > > 2) move it entirely to OnReceivedTransportFeedback() if it will not make > OnReceivedTransportFeedback() too lengthy I'll go with #1. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (left): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:636: // Feedbacks spanning multiple quadrant are treated correctly (Q1-Q2). On 2017/02/17 11:09:14, minyue-webrtc wrote: > On 2017/02/16 15:51:40, elad.alon wrote: > > On 2017/02/16 09:51:44, minyue-webrtc wrote: > > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > > why the tests below here also become obsolete? > > > > > > missed this comment? > > > > Yes, I'd missed the comment; thanks for pointing it out again. > > These tests are no longer relevant. > > * Before, we had no notion of unacked message in the window. When a new > feedback > > arrived, it could trigger moving the window in a way that would get old > messages > > forgotten (independently of the window being full or not). > > * Now, we have the notion of unacked windows in the packet, and it's the > > addition of new packets that could get old ones discarded. That being said, > yes, > > I see the sense in adding a new test where we get 0x8000 or more in between > > unacked messages, and therefore reset the window. > > Ok. then, as you said, the removing of old packets for OnPacketAdded (which I > think it still be OnSentPacket) should be tested. Yes. The following UTs have been added (when reviewing, please note that they're not all in one block, because one of them belongs elsewhere): * EmptyWindowFeedback * UnackedInWindowDoesNotMoveWindow * UnackedOutOfWindowMovesWindow_1 * UnackedOutOfWindowMovesWindow_2 * WindowResetByPacketsTooNew (I think the comments make the difference between UnackedOutOfWindowMovesWindow_1 and UnackedOutOfWindowMovesWindow_2 clear, but let me know if you'd prefer something else.) https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:588: // but does not reset it. On 2017/02/17 11:09:15, minyue-webrtc wrote: > it looks like _2 include _1, since if a packet that is less than 0x8000 from the > newest does not trigger a reset, why should 0x8000 from baseline trigger a > reset. Please note the difference between baseline (oldest acked/unacked) and the acked newest packet. There are three cases here: * Distance from base <= 0x7fff - no reset. (UnackedOutOfWindowMovesWindow_1) * Distance from base >= 0x8000, but distance from newest still <= 0x7fff - no reset. (UnackedOutOfWindowMovesWindow_2) * Distance from base implicitly >= 0x8000, because distance from newest >= 0x8000. (WindowResetAfterLongNoSend) https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:614: // Expected window contents: [] -> [1-GAP-10101]. On 2017/02/17 11:09:15, minyue-webrtc wrote: > This test may be made less complicated, how about > > [10101] -> base + 5 + 0x8000 -> window moved. (like the previous one, which I > think can be removed, though) > > If you find the test not covering enough cases, try divide it into small tests, > each of which is to one point. I think it would be easier to clarify this offline. I'm available for VoIP whenever is convenient for you. https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:632: // Adding more data proves the window was not reset; it still had one On 2017/02/17 11:09:15, minyue-webrtc wrote: > proves -> to prove I think it should be "proves". The action (singular) proves, not the data (plural). https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:642: TEST(TransportFeedbackPacketLossTrackerTest, WindowResetByPacketsTooNew) { On 2017/02/17 11:09:15, minyue-webrtc wrote: > TooNew may not be good. how about WindowResetAfterLongNoSend? Done.
https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:130: PacketStatus new_packet_status) { On 2017/02/17 12:24:42, elad.alon wrote: > On 2017/02/17 11:09:14, minyue-webrtc wrote: > > I see another issue. RecordPacketStatus sounds more generic than its name. > > Because it is called only for feedback handling but it takes PacketStatus as > > input, which can be UNACKED. Therefore one may think of either > > 1) change the scope by making it > > RecordFeedback(PacketStatusIterator it, bool received) > > > > 2) move it entirely to OnReceivedTransportFeedback() if it will not make > > OnReceivedTransportFeedback() too lengthy > > I'll go with #1. By the way, I find the new code to be slightly less clear inside the function, because we now have (A && B) where A is a comparison of PacketStatus and B is a boolean. Before it was clearer, imho, that two successive packets were being examined. I'd be happy to revert this one change if you also feel like that.
Hi, except for the newly added unittest which we can discuss offline, it generally looks good. Thanks! https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:70: void TransportFeedbackPacketLossTracker::OnPacketAdded(uint16_t seq_num) { On 2017/02/17 12:24:42, elad.alon wrote: > On 2017/02/17 11:09:14, minyue-webrtc wrote: > > On 2017/02/16 15:51:40, elad.alon wrote: > > > On 2017/02/16 09:51:44, minyue-webrtc wrote: > > > > On 2017/02/15 16:47:36, elad.alon wrote: > > > > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > > > > The name should be OnSentPacket or something like that IMO > > > > > > > > > > This is triggered from the pre-existing > > TransportFeedbackAdapter::AddPacket > > > > (see > > > > > subsequent CL), which is why I prefer this name. > > > > > > > > Should it hook to OnSentPacket instead: see > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/congestion_co... > > > > > > That one seems to be called after the packet is sent, which creates a race > > with > > > the feedback from the other side. It's a race we're very unlikely to lose, > but > > > it's still a race. > > > > I am not seeing this as a problem. Please refer to my corresponding comment in > > the follow-up cl. > > (Replied there.) Per offline discussion, I think AddPacket is the right API to hook to now, then it is fine to use OnPacketAdded here. https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:129: PacketStatusIterator it, On 2017/02/17 12:24:42, elad.alon wrote: > On 2017/02/17 11:09:14, minyue-webrtc wrote: > > On 2017/02/16 15:51:40, elad.alon wrote: > > > On 2017/02/16 09:51:44, minyue-webrtc wrote: > > > > On 2017/02/15 16:47:36, elad.alon wrote: > > > > > On 2017/02/14 16:46:59, minyue-webrtc wrote: > > > > > > if we make "it" into seq_num instead, we may avoid > > "PacketStatusIterator" > > > > > > > > > > > > Then this function can do > > > > > > > > > > > > 1. find(seq_num) > > > > > > 2. if (new is any of the acks && was not found) { return } > > > > > > 3. what we have here already > > > > > > > > > > > > > > > > I feel that calling RecordPacketStatus() when it might have no effect is > > > > > confusing. I think it's clearer like this. Wdyt? > > > > > > > > I feel typedef a non-const iterator just for this function is not > necessary. > > > I'd > > > > like avoiding iterator, since we cannot guarantee that iterator is always > > > valid. > > > > > > > > If you ask me about ConstPacketStatusIterator, yes, it is good to avoid it > > > too, > > > > but I did not find a way that is as efficient. > > > > > > I feel like RecordPacketStatus() makes the code clearer, so I've kept it. I > > did > > > get rid of the typedef for the non-const iterator. I'll keep the > > > ConstPacketStatusIterator's new name, because I feel that it's clearer. But > > > frankly, it only saves a couple of chars, so I'd like to get rid of it > > > completely, too. Wdyt? > > > > ConstPacketStatusIterator was introduced to avoid duplication, it does appear > a > > lot more often. > > Do you want the name to be reverted to PacketStatusIterator? No ConstPacketStatusIterator is fine https://codereview.webrtc.org/2632203002/diff/380001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:130: PacketStatus new_packet_status) { On 2017/02/17 12:27:03, elad.alon wrote: > On 2017/02/17 12:24:42, elad.alon wrote: > > On 2017/02/17 11:09:14, minyue-webrtc wrote: > > > I see another issue. RecordPacketStatus sounds more generic than its name. > > > Because it is called only for feedback handling but it takes PacketStatus as > > > input, which can be UNACKED. Therefore one may think of either > > > 1) change the scope by making it > > > RecordFeedback(PacketStatusIterator it, bool received) > > > > > > 2) move it entirely to OnReceivedTransportFeedback() if it will not make > > > OnReceivedTransportFeedback() too lengthy > > > > I'll go with #1. > > By the way, I find the new code to be slightly less clear inside the function, > because we now have (A && B) where A is a comparison of PacketStatus and B is a > boolean. Before it was clearer, imho, that two successive packets were being > examined. I'd be happy to revert this one change if you also feel like that. I like the boolean ssince it is more important for the function signature to be clearer. https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:614: // Expected window contents: [] -> [1-GAP-10101]. On 2017/02/17 12:24:42, elad.alon wrote: > On 2017/02/17 11:09:15, minyue-webrtc wrote: > > This test may be made less complicated, how about > > > > [10101] -> base + 5 + 0x8000 -> window moved. (like the previous one, which I > > think can be removed, though) > > > > If you find the test not covering enough cases, try divide it into small > tests, > > each of which is to one point. > > I think it would be easier to clarify this offline. I'm available for VoIP > whenever is convenient for you. let's do https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:632: // Adding more data proves the window was not reset; it still had one On 2017/02/17 12:24:42, elad.alon wrote: > On 2017/02/17 11:09:15, minyue-webrtc wrote: > > proves -> to prove > > I think it should be "proves". The action (singular) proves, not the data > (plural). not grammar. I mean the sentence is to indicates the purpose of the following action. So it is actually "adding more data (in order) to prove ... " The true problem is that "adding more data" cannot "prove the window was not reset". It is "adding more data and verifying it still has one ..." that "proves" ...
lgtm on PS24. I now ping other reviewers to review and approve.
https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (left): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:52: Reset(); Calling a function called "Reset" in the ctor sounds strange to me. To which state will the class be set ? So we may rename the function to "SetInitState" or "SetZeroState" .. or something else :) https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:167: void TransportFeedbackPacketLossTracker::UpdatePlr( Would be nice to move this function to PlrState. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:246: void TransportFeedbackPacketLossTracker::Validate() const { // Testing only! Do we have any concrete conditions when we will remove this function ? https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:193: default: A question to all: In my opinion its not necessary to check the default case of a enum class. What do you think about this ? https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: ConstPacketStatusIterator it, Would be nice to move this function to RplrState. We could pass PreviousPacketStatus and NextPacketStatus to make to function not dependent on TransportFeedbackPacketLossTracker. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (left): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:102: Reset(); Calling a function called "Reset" in the ctor sounds strange to me. To which state will the class be set ? So we may rename the function to "SetInitState" or "SetZeroState" .. or something else :) We may even remove the reset here and just recreate the class.
https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2632203002/diff/420001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:632: // Adding more data proves the window was not reset; it still had one On 2017/02/20 12:49:06, minyue-webrtc wrote: > On 2017/02/17 12:24:42, elad.alon wrote: > > On 2017/02/17 11:09:15, minyue-webrtc wrote: > > > proves -> to prove > > > > I think it should be "proves". The action (singular) proves, not the data > > (plural). > > not grammar. I mean the sentence is to indicates the purpose of the following > action. So it is actually "adding more data (in order) to prove ... " > > The true problem is that "adding more data" cannot "prove the window was not > reset". It is "adding more data and verifying it still has one ..." that > "proves" ... Acknowledged. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (left): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:52: Reset(); On 2017/02/27 12:44:21, michaelt wrote: > Calling a function called "Reset" in the ctor sounds strange to me. > To which state will the class be set ? > So we may rename the function to "SetInitState" or "SetZeroState" .. or > something else :) > You voice legitimate concerns which were discussed in preceding CLs. The conclusion was to keep Reset() here, but get rid of it for plr_state_ and rplr_state_ (which doesn't really work that well without a std::unique_ptr, so we've let that go for now; I can refer you to the previous discussion of this if you'd like). Minyue, I assume this is still your preference? I don't mind changing otherwise, but my two cents are that Reset() is clear enough. Please note that Reset() is also used from places except the ctor, which is why it makes sense to have a function for it. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:167: void TransportFeedbackPacketLossTracker::UpdatePlr( On 2017/02/27 12:44:21, michaelt wrote: > Would be nice to move this function to PlrState. > I agree in principle. Let's keep the discussion around UpdateRplr(), which is the problematic case, as you've noticed, and then stay symmetric - move either both or neither. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:246: void TransportFeedbackPacketLossTracker::Validate() const { // Testing only! On 2017/02/27 12:44:21, michaelt wrote: > Do we have any concrete conditions when we will remove this function ? The fuzzer relies heavily on this, so I don't see us removing this function unless we remove the fuzzer. Also, the UTs make use of this. I'd be happy to keep this in the code. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:193: default: On 2017/02/27 12:44:21, michaelt wrote: > A question to all: > In my opinion its not necessary to check the default case of a enum class. > What do you think about this ? +1 question (I think checking that is generally safer, but if the rest of the codebase considers this overkill, there's no need for me to diverge from convention in this case, which does not involve potential input from other modules, etc., which would have created a stronger need for extra checks.) https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: ConstPacketStatusIterator it, On 2017/02/27 12:44:21, michaelt wrote: > Would be nice to move this function to RplrState. > We could pass PreviousPacketStatus and NextPacketStatus to make to function not > dependent on TransportFeedbackPacketLossTracker. As you've noticed, this is the problematic case. We want to minimize the struct's visibility into the data-structure which it is tracking (SentPacketStatusMap). If a previous/next always existed, we could just give both statuses to the function, and that would be it. Because the existence of previous/next is not guaranteed, we need to give an iterator, and that exposes too much, I feel. There are ways around - rtc::Optional, pointer, etc. To me they seem overkill, but I'm open to discussion. Do you have a suggested solution? Off the top of my head, rtc::Optional seems the cleanest, but it also means relative inefficiency. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (left): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:102: Reset(); On 2017/02/27 12:44:21, michaelt wrote: > Calling a function called "Reset" in the ctor sounds strange to me. > To which state will the class be set ? > So we may rename the function to "SetInitState" or "SetZeroState" .. or > something else :) > We may even remove the reset here and just recreate the class. This was suggested before. Unlike TransportFeedbackPacketLossTracker::Reset(), where the conclusion was to keep the Reset() function, here the decision was to remove and recreate. However, we don't want to do this using a unique_ptr, which would mean incurring a penalty due to pointer dereferencing (potential cache-miss) whenever accessing the object, and using the assignment operator got complaints from the compiler. Therefore, I abandoned this idea and kept things as they are. References to previous discussions on the topic: 1. Previous discussion about Reset(): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... 2. Compiler doesn't allow implicit use of assignment operator for this class; would require explicit definition: https://codereview.webrtc.org/2632203002/diff/320001/webrtc/voice_engine/tran... (Note: I'd argue that if the assignment operator needs to be explicitly defined, it's not as good as Reset(), which, unlike the assignment operator, can be used from the ctor, too.)
https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (left): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:52: Reset(); Hmm I think i would go for the std::unique_ptr solution here. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:246: void TransportFeedbackPacketLossTracker::Validate() const { // Testing only! I just asked because minyue wrote in the comment "Once the fuzzer test shows no error after long period, we can remove the fuzzer test, and move this method to unit test." which seams to be quite a open criteria :). I may have to clarify this with him. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: ConstPacketStatusIterator it, I think i would go for the "rtc::Optional" solution.
https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (left): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:52: Reset(); On 2017/02/27 15:46:18, michaelt wrote: > Hmm I think i would go for the std::unique_ptr solution here. > > That would require the tracker informing its owner that it needs to be replaced by a new instance, which I think would be bad design. Or is this comment actually about |plr_state_| and |rplr_state_|, but not about the object for which this is the ctor? https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:246: void TransportFeedbackPacketLossTracker::Validate() const { // Testing only! On 2017/02/27 15:46:18, michaelt wrote: > I just asked because minyue wrote in the comment "Once the fuzzer test shows no > error after long period, we can remove the fuzzer test, and move this method to > unit test." which seams to be quite a open criteria :). > I may have to clarify this with him. > Ah. Well, if he wants to remove this, of course. :-) Personally, I'd vote for keeping this, but I can see the merit of either decision. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: ConstPacketStatusIterator it, On 2017/02/27 15:46:18, michaelt wrote: > I think i would go for the "rtc::Optional" solution. > Okay. I'll wait for feedback from Erik and/or Stefan before implementing.
https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (left): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:52: Reset(); :) no not for the tracker :) For plr and rplr states. Then add acked_packets_ and ref_packet_status_ to the ctor of the tracker and remove the reset form the ctor. (sorry the last comment was quite misleading)
https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (left): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:52: Reset(); On 2017/02/27 16:04:31, michaelt wrote: > :) no not for the tracker :) > For plr and rplr states. > Then add acked_packets_ and ref_packet_status_ to the ctor of the tracker and > remove the reset form the ctor. > > (sorry the last comment was quite misleading) It's possible for the plr_state_ and rplr_state_, but I think the associated overhead makes it undesirable. It's only rarely that the objects are reset anytime besides in the ctor (we expect this to never happen in ~99.9% of the calls), and the pointer would mean lesser efficiency whenever accessed (100% of calls). Let's see what other people think?
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2632203002/#ps560001 (title: ".")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14439)
https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (left): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:52: Reset(); On 2017/02/27 16:29:19, elad.alon wrote: > On 2017/02/27 16:04:31, michaelt wrote: > > :) no not for the tracker :) > > For plr and rplr states. > > Then add acked_packets_ and ref_packet_status_ to the ctor of the tracker and > > remove the reset form the ctor. > > > > (sorry the last comment was quite misleading) > > It's possible for the plr_state_ and rplr_state_, but I think the associated > overhead makes it undesirable. It's only rarely that the objects are reset > anytime besides in the ctor (we expect this to never happen in ~99.9% of the > calls), and the pointer would mean lesser efficiency whenever accessed (100% of > calls). Let's see what other people think? Hi, I hoped to get rid of Reset() everywhere, but agreed that we could have it everywhere like this. But I am fine if we try to remove all Reset() in any separate CL. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:246: void TransportFeedbackPacketLossTracker::Validate() const { // Testing only! On 2017/02/27 15:53:32, elad.alon wrote: > On 2017/02/27 15:46:18, michaelt wrote: > > I just asked because minyue wrote in the comment "Once the fuzzer test shows > no > > error after long period, we can remove the fuzzer test, and move this method > to > > unit test." which seams to be quite a open criteria :). > > I may have to clarify this with him. > > > > Ah. Well, if he wants to remove this, of course. :-) Personally, I'd vote for > keeping this, but I can see the merit of either decision. Let's keep it until the feature has been fully landed in products, and no algorithmic changes are ahead of us. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:193: default: On 2017/02/27 13:18:05, elad.alon wrote: > On 2017/02/27 12:44:21, michaelt wrote: > > A question to all: > > In my opinion its not necessary to check the default case of a enum class. > > What do you think about this ? > > +1 question > (I think checking that is generally safer, but if the rest of the codebase > considers this overkill, there's no need for me to diverge from convention in > this case, which does not involve potential input from other modules, etc., > which would have created a stronger need for extra checks.) Agree. compiling warnings should be enough. Please remove, unless someone knows a reason to keep it. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: ConstPacketStatusIterator it, On 2017/02/27 15:53:32, elad.alon wrote: > On 2017/02/27 15:46:18, michaelt wrote: > > I think i would go for the "rtc::Optional" solution. > > > > Okay. I'll wait for feedback from Erik and/or Stefan before implementing. I would also like to make PlrState and RplrState expose fewer accessibility. Putting these into them and make their data members private will be nice. But then they should become class rather than struct. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... May do it in a separate CL.
https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: ConstPacketStatusIterator it, On 2017/03/03 14:14:31, minyue-webrtc wrote: > On 2017/02/27 15:53:32, elad.alon wrote: > > On 2017/02/27 15:46:18, michaelt wrote: > > > I think i would go for the "rtc::Optional" solution. > > > > > > > Okay. I'll wait for feedback from Erik and/or Stefan before implementing. > > I would also like to make PlrState and RplrState expose fewer accessibility. > Putting these into them and make their data members private will be nice. But > then they should become class rather than struct. > > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > May do it in a separate CL. That links is not publicly assessible, see this instead. https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:193: default: On 2017/03/03 14:14:31, minyue-webrtc wrote: > On 2017/02/27 13:18:05, elad.alon wrote: > > On 2017/02/27 12:44:21, michaelt wrote: > > > A question to all: > > > In my opinion its not necessary to check the default case of a enum class. > > > What do you think about this ? > > > > +1 question > > (I think checking that is generally safer, but if the rest of the codebase > > considers this overkill, there's no need for me to diverge from convention in > > this case, which does not involve potential input from other modules, etc., > > which would have created a stronger need for extra checks.) > > Agree. compiling warnings should be enough. Please remove, unless someone knows > a reason to keep it. Discussed this with Minyue offline, and he'd like to keep the defensive approach. There are two ways this could be triggered: 1. Receive an unsanitized value from outside (cast to enum of something from network/file/user). 2. Memory corruption in another module ends up passing us a bad value, and we fail to catch it. https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: ConstPacketStatusIterator it, On 2017/03/03 14:14:31, minyue-webrtc wrote: > On 2017/02/27 15:53:32, elad.alon wrote: > > On 2017/02/27 15:46:18, michaelt wrote: > > > I think i would go for the "rtc::Optional" solution. > > > > > > > Okay. I'll wait for feedback from Erik and/or Stefan before implementing. > > I would also like to make PlrState and RplrState expose fewer accessibility. > Putting these into them and make their data members private will be nice. But > then they should become class rather than struct. > > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > May do it in a separate CL. So no action for now?
On 2017/03/03 14:54:22, elad.alon wrote: > https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... > File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): > > https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... > webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:193: default: > On 2017/03/03 14:14:31, minyue-webrtc wrote: > > On 2017/02/27 13:18:05, elad.alon wrote: > > > On 2017/02/27 12:44:21, michaelt wrote: > > > > A question to all: > > > > In my opinion its not necessary to check the default case of a enum class. > > > > What do you think about this ? > > > > > > +1 question > > > (I think checking that is generally safer, but if the rest of the codebase > > > considers this overkill, there's no need for me to diverge from convention > in > > > this case, which does not involve potential input from other modules, etc., > > > which would have created a stronger need for extra checks.) > > > > Agree. compiling warnings should be enough. Please remove, unless someone > knows > > a reason to keep it. > > Discussed this with Minyue offline, and he'd like to keep the defensive > approach. There are two ways this could be triggered: > 1. Receive an unsanitized value from outside (cast to enum of something from > network/file/user). > 2. Memory corruption in another module ends up passing us a bad value, and we > fail to catch it. > > https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... > webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: > ConstPacketStatusIterator it, > On 2017/03/03 14:14:31, minyue-webrtc wrote: > > On 2017/02/27 15:53:32, elad.alon wrote: > > > On 2017/02/27 15:46:18, michaelt wrote: > > > > I think i would go for the "rtc::Optional" solution. > > > > > > > > > > Okay. I'll wait for feedback from Erik and/or Stefan before implementing. > > > > I would also like to make PlrState and RplrState expose fewer accessibility. > > Putting these into them and make their data members private will be nice. But > > then they should become class rather than struct. > > > > > > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > > > May do it in a separate CL. > > So no action for now? I don't think so. Let this CL focus on stream separation. (I think there are already some not-so-related refactoring, which made the review a big harder)
On 2017/03/03 14:56:00, minyue-webrtc wrote: > On 2017/03/03 14:54:22, elad.alon wrote: > > > https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... > > File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): > > > > > https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... > > webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:193: default: > > On 2017/03/03 14:14:31, minyue-webrtc wrote: > > > On 2017/02/27 13:18:05, elad.alon wrote: > > > > On 2017/02/27 12:44:21, michaelt wrote: > > > > > A question to all: > > > > > In my opinion its not necessary to check the default case of a enum > class. > > > > > What do you think about this ? > > > > > > > > +1 question > > > > (I think checking that is generally safer, but if the rest of the codebase > > > > considers this overkill, there's no need for me to diverge from convention > > in > > > > this case, which does not involve potential input from other modules, > etc., > > > > which would have created a stronger need for extra checks.) > > > > > > Agree. compiling warnings should be enough. Please remove, unless someone > > knows > > > a reason to keep it. > > > > Discussed this with Minyue offline, and he'd like to keep the defensive > > approach. There are two ways this could be triggered: > > 1. Receive an unsanitized value from outside (cast to enum of something from > > network/file/user). > > 2. Memory corruption in another module ends up passing us a bad value, and we > > fail to catch it. > > > > > https://codereview.webrtc.org/2632203002/diff/560001/webrtc/voice_engine/tran... > > webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: > > ConstPacketStatusIterator it, > > On 2017/03/03 14:14:31, minyue-webrtc wrote: > > > On 2017/02/27 15:53:32, elad.alon wrote: > > > > On 2017/02/27 15:46:18, michaelt wrote: > > > > > I think i would go for the "rtc::Optional" solution. > > > > > > > > > > > > > Okay. I'll wait for feedback from Erik and/or Stefan before implementing. > > > > > > I would also like to make PlrState and RplrState expose fewer accessibility. > > > Putting these into them and make their data members private will be nice. > But > > > then they should become class rather than struct. > > > > > > > > > > > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > > > > > May do it in a separate CL. > > > > So no action for now? > > I don't think so. Let this CL focus on stream separation. (I think there are > already some not-so-related refactoring, which made the review a big harder) RS LGTM
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2632203002/#ps580001 (title: "Stricter compiler on server; appeased?")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14468)
elad.alon@webrtc.org changed reviewers: + pbos@webrtc.org
pbos@, could you please lgtm this? We need you as an owner of the fuzzer directory. :-)
rs-lgtm for fuzzing directory, I'm assuming this part has been reviewed. Otherwise have someone review it before submitting it.
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2632203002/#ps600001 (title: ".")
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": 600001, "attempt_start_ts": 1488565973644550,
"parent_rev": "b85a88853a651a248078194089e9d5563a41350c", "commit_rev":
"7af935758037dc483581ea701624cdb1a4dc77af"}
Message was sent while issue was closed.
Description was changed from ========== Packet Loss Tracker - Stream Separation 1. Packet reception statistics (PLR and RPLR) calculated on each stream separately. 2. Algorithm changed from considering separate quadrants of the sequence-number-ring to simply looking at the oldest in-window SENT packet. BUG=webrtc:7058 ========== to ========== Packet Loss Tracker - Stream Separation 1. Packet reception statistics (PLR and RPLR) calculated on each stream separately. 2. Algorithm changed from considering separate quadrants of the sequence-number-ring to simply looking at the oldest in-window SENT packet. BUG=webrtc:7058 Review-Url: https://codereview.webrtc.org/2632203002 Cr-Commit-Position: refs/heads/master@{#17018} Committed: https://chromium.googlesource.com/external/webrtc/+/7af935758037dc483581ea701... ==========
Message was sent while issue was closed.
Committed patchset #31 (id:600001) as https://chromium.googlesource.com/external/webrtc/+/7af935758037dc483581ea701... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
