Re: [quicwg/base-drafts] Refactor DetectLostPackets (#2066)

[janaiyengar <[e-mail]>]

janaiyengar commented on this pull request.

Some comments, but I think this refactor is basically correct (mod the corrections I've noted), and is definitely helpful! @ianswett should take a look as well.

>    else if (largest_acked.packet_number == largest_sent_packet):
     // Early retransmit timer.
-    delay_until_lost = 9/8 * max(latest_rtt, smoothed_rtt)
-  foreach (unacked < largest_acked.packet_number):
-    time_since_sent = now() - unacked.time_sent
-    delta = largest_acked.packet_number - unacked.packet_number
-    if (time_since_sent > delay_until_lost ||
-        delta > reordering_threshold):
+    loss_delay = 9/8 * max(latest_rtt, smoothed_rtt)

This can be (1 + time_reordering_fraction)

> -  foreach (unacked < largest_acked.packet_number):
-    time_since_sent = now() - unacked.time_sent
-    delta = largest_acked.packet_number - unacked.packet_number
-    if (time_since_sent > delay_until_lost ||
-        delta > reordering_threshold):
+    loss_delay = 9/8 * max(latest_rtt, smoothed_rtt)
+  // Packets sent before this time are deemed lost.
+  lost_send_time -= loss_delay
+  lost_pn = largest_acked.packet_number - reordering_threshold
+
+  foreach unacked:
+    if unacked.packet_number > largest_acked.packet_number:
+      continue
+
+    if (unacked.time_sent > lost_send_time ||
+        unacked.packet_number > lost_pn):

This should be
```
    if (lost_send_time > unacked.time_sent || lost_pn > unacked.packet_number)
```

>  
   // Inform the congestion controller of lost packets and
   // lets it decide whether to retransmit immediately.
   if (!lost_packets.empty()):
     OnPacketsLost(lost_packets)
 ~~~
 
+This algorithm results in loss_time being set to the earliest time that the
+earliest packet was sent.  As a result loss_time could be in the past.  Timers

This algorithm results in loss_time being set based on the earliest packet that is still in flight. As a result ...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066#pullrequestreview-179615473