Adding transmission times for every QUIC packet received in the AckFrame.

net/quic/quic_framer.cc

Index: net/quic/quic_framer.cc
diff --git a/net/quic/quic_framer.cc b/net/quic/quic_framer.cc
index 708bd7576bedb9136749ed323aa351621e2337c5..2a9b0a12d57e7560b118c13a582758cf085a58ad 100644
--- a/net/quic/quic_framer.cc
+++ b/net/quic/quic_framer.cc
@@ -12,6 +12,7 @@
 #include "net/quic/quic_utils.h"
 
 using base::StringPiece;
+using std::map;
 
 namespace net {
 
@@ -353,31 +354,46 @@ bool QuicFramer::ProcessPDUFrame() {
 }
 
 bool QuicFramer::ProcessAckFrame(QuicAckFrame* frame) {
-  if (!reader_->ReadUInt48(&frame->received_info.largest_received)) {
-    set_detailed_error("Unable to read largest received.");
+  uint8 num_acked_packets;
+  if (!reader_->ReadBytes(&num_acked_packets, 1)) {
+    set_detailed_error("Unable to read num acked packets.");
     return false;
   }
-  uint64 time_received;
-  if (!reader_->ReadUInt64(&time_received)) {
-    set_detailed_error("Unable to read time received.");
+
+  uint64 smallest_received;
+  if (!reader_->ReadUInt48(&smallest_received)) {
+    set_detailed_error("Unable to read smallest received.");
     return false;
   }
-  frame->received_info.time_received =
-      QuicTime::FromMicroseconds(time_received);
 
-  uint8 num_unacked_packets;
-  if (!reader_->ReadBytes(&num_unacked_packets, 1)) {
-    set_detailed_error("Unable to read num unacked packets.");
+  if (num_acked_packets == 0u) {

[jar (doing other things), 2012/11/30 16:50:33]

Do we really ever send a 0 for the number of acked packets?

[Ian Swett, 2012/12/04 21:40:19]

It would seem very odd in practice, and right now we're acking every packet, so
no.  If no packets were recently received, and there were still pending nacks,
would we want to re-send an ack with the nacks?

+    // Ensures largest_received is set when no actual acks are transmitted.
+    frame->received_info.largest_received = smallest_received;
+  } else {
+    uint64 time_received_us;
+    if (!reader_->ReadUInt64(&time_received_us)) {
+      set_detailed_error("Unable to read time received.");
     return false;

[jar (doing other things), 2012/11/30 16:50:33]

nit: indent by two more spaces.

[Ryan Hamilton, 2012/12/04 18:04:51]

Weird, this appears to be a merge problem somehow.  Fixed.

   }

[jar (doing other things), 2012/11/30 16:50:33]

nit: indent by two more spaces.

[Ryan Hamilton, 2012/12/04 18:04:51]

Done.

 
-  for (int i = 0; i < num_unacked_packets; ++i) {
-    QuicPacketSequenceNumber sequence_number;
-    if (!reader_->ReadUInt48(&sequence_number)) {
-      set_detailed_error("Unable to read sequence number in unacked packets.");
+    frame->received_info.RecordAck(
+        smallest_received, QuicTime::FromMicroseconds(time_received_us));
+
+    for (int i = 0; i < num_acked_packets - 1; ++i) {
+      uint8 sequence_delta;
+      if (!reader_->ReadBytes(&sequence_delta, 1)) {
+        set_detailed_error("Unable to read sequence delta in acked packets.");
+        return false;
+      }
+      int32 time_delta_us;
+      if (!reader_->ReadBytes(&time_delta_us, sizeof(time_delta_us))) {
+        set_detailed_error("Unable to read time delta in acked packets.");
       return false;

[jar (doing other things), 2012/11/30 16:50:33]

nit: indent by two more spaces.

[Ryan Hamilton, 2012/12/04 18:04:51]

Done.

     }

[jar (doing other things), 2012/11/30 16:50:33]

nit: indent by two more spaces.

[Ryan Hamilton, 2012/12/04 18:04:51]

Done.

-    frame->received_info.missing_packets.insert(sequence_number);
+      frame->received_info.RecordAck(
+          smallest_received + sequence_delta,
+          QuicTime::FromMicroseconds(time_received_us + time_delta_us));
+    }
   }
 
   if (!reader_->ReadUInt48(&frame->sent_info.least_unacked)) {
@@ -385,21 +401,6 @@ bool QuicFramer::ProcessAckFrame(QuicAckFrame* frame) {
     return false;
   }
 
-  uint8 num_non_retransmiting_packets;
-  if (!reader_->ReadBytes(&num_non_retransmiting_packets, 1)) {
-    set_detailed_error("Unable to read num non-retransmitting.");
-    return false;
-  }
-  for (uint8 i = 0; i < num_non_retransmiting_packets; ++i) {
-    QuicPacketSequenceNumber sequence_number;
-    if (!reader_->ReadUInt48(&sequence_number)) {
-      set_detailed_error(
-          "Unable to read sequence number in non-retransmitting.");
-      return false;
-    }
-    frame->sent_info.non_retransmiting.insert(sequence_number);
-  }
-
   uint8 congestion_info_type;
   if (!reader_->ReadBytes(&congestion_info_type, 1)) {
     set_detailed_error("Unable to read congestion info type.");
@@ -582,13 +583,13 @@ size_t QuicFramer::ComputeFramePayloadLength(const QuicFrame& frame) {
       break;  // Need to support this eventually :>
     case ACK_FRAME: {
       const QuicAckFrame& ack = *frame.ack_frame;
+      len += 1;  // num acked packets
       len += 6;  // largest received packet sequence number
-      len += 8;  // time delta
-      len += 1;  // num missing packets
-      len += 6 * ack.received_info.missing_packets.size();
+      if (ack.received_info.received_packet_times.size() > 0) {
+        len += 8;  // time
+        len += 5 * (ack.received_info.received_packet_times.size() - 1);

[jar (doing other things), 2012/11/30 16:50:33]

nit: add comment about what is included in this product.

[Ian Swett, 2012/12/04 21:40:19]

Done in cleanup CL.

+      }
       len += 6;  // least packet sequence number awaiting an ack
-      len += 1;  // num non retransmitting packets
-      len += 6 * ack.sent_info.non_retransmiting.size();
       len += 1;  // congestion control type
       switch (ack.congestion_info.type) {
         case kNone:
@@ -656,44 +657,47 @@ bool QuicFramer::AppendStreamFramePayload(
 bool QuicFramer::AppendAckFramePayload(
     const QuicAckFrame& frame,
     QuicDataWriter* writer) {
-  if (!writer->WriteUInt48(frame.received_info.largest_received)) {
+  uint8 num_acked_packets = frame.received_info.received_packet_times.size();
+  if (!writer->WriteBytes(&num_acked_packets, 1)) {
     return false;
   }
-
-  if (!writer->WriteUInt64(
-      frame.received_info.time_received.ToMicroseconds())) {
+  if (num_acked_packets == 0) {
+    // Special case when no packets are acked, just transmit the largest.

[jar (doing other things), 2012/11/30 16:50:33]

I'm confused about why this is needed/used.

[Ian Swett, 2012/12/04 21:40:19]

Agreed, fixed in HEAD.

+    if (!writer->WriteUInt48(frame.received_info.largest_received)) {
     return false;
   }

[jar (doing other things), 2012/11/30 16:50:33]

nit: indent last two lines.

[Ryan Hamilton, 2012/12/04 18:04:51]

codereview seems to be confused.  This is correctly indented in the source file.
 *boggle*

+  } else {
+    map<QuicPacketSequenceNumber, QuicTime>::const_iterator it =
+        frame.received_info.received_packet_times.begin();
 
-  size_t num_unacked_packets = frame.received_info.missing_packets.size();
-  if (!writer->WriteBytes(&num_unacked_packets, 1)) {
+    QuicPacketSequenceNumber lowest_sequence = it->first;
+    if (!writer->WriteUInt48(lowest_sequence)) {
     return false;
   }

[jar (doing other things), 2012/11/30 16:50:33]

nit: indent last two lines.

Is there any chance this is the result of a bad merge?... or just an evil
search/replace?

[Ryan Hamilton, 2012/12/04 18:04:51]

This is also correct in the file.  Not sure why codereview is confused.

 
-  SequenceSet::const_iterator it = frame.received_info.missing_packets.begin();
-  for (; it != frame.received_info.missing_packets.end(); ++it) {
-    if (!writer->WriteUInt48(*it)) {
+    QuicTime lowest_time = it->second;
+    // TODO(ianswett): Use time deltas from the connection's first received
+    // packet.
+    if (!writer->WriteUInt64(lowest_time.ToMicroseconds())) {
       return false;
     }
-  }
 
-  if (!writer->WriteUInt48(frame.sent_info.least_unacked)) {
+    for (++it; it != frame.received_info.received_packet_times.end(); ++it) {
+      QuicPacketSequenceNumber sequence_delta = it->first - lowest_sequence;
+      if (!writer->WriteBytes(&sequence_delta, 1)) {
     return false;
   }

[jar (doing other things), 2012/11/30 16:50:33]

nit: indent last 2 lines by 4 more spaces.

[Ryan Hamilton, 2012/12/04 18:04:51]

ditto.

 
-  size_t num_non_retransmiting_packets =
-      frame.sent_info.non_retransmiting.size();
-  if (!writer->WriteBytes(&num_non_retransmiting_packets, 1)) {
+      int32 time_delta_us = it->second.Subtract(lowest_time).ToMicroseconds();
+      if (!writer->WriteBytes(&time_delta_us, sizeof(time_delta_us))) {
     return false;
   }

[jar (doing other things), 2012/11/30 16:50:33]

nit: indent last two lines.

[Ryan Hamilton, 2012/12/04 18:04:51]

ditto

+    }
+  }
 
-  it = frame.sent_info.non_retransmiting.begin();
-  while (it != frame.sent_info.non_retransmiting.end()) {
-    if (!writer->WriteUInt48(*it)) {
+  if (!writer->WriteUInt48(frame.sent_info.least_unacked)) {
       return false;
     }
-    ++it;
-  }
 
   if (!writer->WriteBytes(&frame.congestion_info.type, 1)) {
     return false;