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

net/quic/quic_framer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/quic_framer.h"
 
 #include "base/hash_tables.h"
 #include "net/quic/crypto/quic_decrypter.h"
 #include "net/quic/crypto/quic_encrypter.h"
 #include "net/quic/quic_data_reader.h"
 #include "net/quic/quic_data_writer.h"
 #include "net/quic/quic_utils.h"
 
 using base::StringPiece;
+using std::map;
 
 namespace net {
 
 QuicFramer::QuicFramer(QuicDecrypter* decrypter, QuicEncrypter* encrypter)
     : visitor_(NULL),
       fec_builder_(NULL),
       error_(QUIC_NO_ERROR),
       decrypter_(decrypter),
       encrypter_(encrypter) {
 }
@@ skipping 321 matching lines @@
 
   visitor_->OnStreamFrame(frame);
   return true;
 }
 
 bool QuicFramer::ProcessPDUFrame() {
   return false;
 }
 
 bool QuicFramer::ProcessAckFrame(QuicAckFrame* frame) {
-  if (!reader_->ReadUInt48(&frame->received_info.largest_received)) {
-    set_detailed_error("Unable to read largest received.");
-    return false;
-  }
-  uint64 time_received;
-  if (!reader_->ReadUInt64(&time_received)) {
-    set_detailed_error("Unable to read time 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.");
+  uint8 num_acked_packets;
+  if (!reader_->ReadBytes(&num_acked_packets, 1)) {
+    set_detailed_error("Unable to read num acked packets.");
     return false;
   }
 
-  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.");
+  uint64 smallest_received;
+  if (!reader_->ReadUInt48(&smallest_received)) {
+    set_detailed_error("Unable to read smallest received.");
+    return false;
+  }
+
+  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.

+
+    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)) {
     set_detailed_error("Unable to read least unacked.");
     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.");
     return false;
   }
   frame->congestion_info.type =
       static_cast<CongestionFeedbackType>(congestion_info_type);
 
   switch (frame->congestion_info.type) {
     case kNone:
@@ skipping 162 matching lines @@
       len += 1;  // fin
       len += 8;  // offset
       len += 2;  // space for the 16 bit length
       len += frame.stream_frame->data.size();
       break;
     case PDU_FRAME:
       DLOG(INFO) << "PDU_FRAME not yet supported";
       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:
           break;
         case kInterArrival:
           len += 6;
           break;
         case kFixRate:
           len += 4;
           break;
@@ skipping 47 matching lines @@
   if (!writer->WriteBytes(frame.data.data(),
                            frame.data.size())) {
     return false;
   }
   return true;
 }
 
 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 (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();
+
+    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.

 
-  if (!writer->WriteUInt64(
-      frame.received_info.time_received.ToMicroseconds())) {
+    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;
+    }
+
+    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_unacked_packets = frame.received_info.missing_packets.size();
-  if (!writer->WriteBytes(&num_unacked_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

-
-  SequenceSet::const_iterator it = frame.received_info.missing_packets.begin();
-  for (; it != frame.received_info.missing_packets.end(); ++it) {
-    if (!writer->WriteUInt48(*it)) {
-      return false;
     }
   }
 
   if (!writer->WriteUInt48(frame.sent_info.least_unacked)) {
-    return false;
-  }
-
-  size_t num_non_retransmiting_packets =
-      frame.sent_info.non_retransmiting.size();
-  if (!writer->WriteBytes(&num_non_retransmiting_packets, 1)) {
-    return false;
-  }
-
-  it = frame.sent_info.non_retransmiting.begin();
-  while (it != frame.sent_info.non_retransmiting.end()) {
-    if (!writer->WriteUInt48(*it)) {
       return false;
     }
-    ++it;
-  }
 
   if (!writer->WriteBytes(&frame.congestion_info.type, 1)) {
     return false;
   }
 
   switch (frame.congestion_info.type) {
     case kNone:
       break;
     case kInterArrival: {
       const CongestionFeedbackMessageInterArrival& inter_arrival =
@@ skipping 72 matching lines @@
 
 bool QuicFramer::RaiseError(QuicErrorCode error) {
   DLOG(INFO) << detailed_error_;
   set_error(error);
   visitor_->OnError(this);
   reader_.reset(NULL);
   return false;
 }
 
 }  // namespace net