Refactoring in RTP reader code: sequence number wrapping logic moved to

RtpReader

BUG=None
TEST=Unittests

Review URL: http://codereview.chromium.org/5110008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@67212 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 114 insertions, 35 deletions

diff --git a/remoting/protocol/rtp_reader.cc b/remoting/protocol/rtp_reader.cc
index 7e21250..f36dc8b 100644
--- a/remoting/protocol/rtp_reader.cc
+++ b/remoting/protocol/rtp_reader.cc
@@ -10,12 +10,25 @@
 namespace remoting {
 namespace protocol {
 
-RtpPacket::RtpPacket() {}
-RtpPacket::~RtpPacket() {}
+namespace {
+// Recomended values from RTP spec.
+const int kMaxDropout = 3000;
+const int kMaxMisorder = 100;
+}  // namespace
+
+RtpPacket::RtpPacket() { }
+
+RtpPacket::~RtpPacket() { }
 
 // RtpReader class.
-RtpReader::RtpReader() {}
-RtpReader::~RtpReader() {}
+RtpReader::RtpReader()
+    : max_sequence_number_(0),
+      wrap_around_count_(0),
+      started_(false) {
+}
+
+RtpReader::~RtpReader() {
+}
 
 void RtpReader::Init(net::Socket* socket,
                      OnMessageCallback* on_message_callback) {
@@ -44,6 +57,32 @@ void RtpReader::OnDataReceived(net::IOBuffer* buffer, int data_size) {
       buffer, buffer->data() + header_size + descriptor_size,
       data_size - header_size - descriptor_size);
 
+  uint16 sequence_number = packet->header().sequence_number;
+
+  // Reset |max_sequence_number_| after we've received first packet.
+  if (!started_) {
+    started_ = true;
+    max_sequence_number_ = sequence_number;
+  }
+
+  int16 delta = sequence_number - max_sequence_number_;
+  if (delta <= -kMaxMisorder || delta > kMaxDropout) {
+    // TODO(sergeyu): Do we need to handle restarted trasmission?
+    LOG(WARNING) << "Received RTP packet with invalid sequence number.";
+    delete packet;
+    return;
+  }
+
+  packet->set_extended_sequence_number(
+      (wrap_around_count_ << 16) + max_sequence_number_ + delta);
+
+  if (delta > 0 && delta < kMaxDropout) {
+    if (sequence_number < max_sequence_number_) {
+      wrap_around_count_++;
+    }
+    max_sequence_number_ = sequence_number;
+  }
+
   on_message_callback_->Run(packet);
 }
 
diff --git a/remoting/protocol/rtp_reader.h b/remoting/protocol/rtp_reader.h
index 1501a3b..1af52b6 100644
--- a/remoting/protocol/rtp_reader.h
+++ b/remoting/protocol/rtp_reader.h
@@ -27,12 +27,24 @@ class RtpPacket {
   const CompoundBuffer& payload() const { return payload_; }
   CompoundBuffer* mutable_payload() { return &payload_; }
 
+  const uint32 extended_sequence_number() const {
+    return extended_sequence_number_;
+  }
+  void set_extended_sequence_number(uint32 value) {
+    extended_sequence_number_ = value;
+  }
+
  private:
   RtpHeader header_;
   CompoundBuffer payload_;
   Vp8Descriptor vp8_descriptor_;
+  uint32 extended_sequence_number_;
 };
 
+// RtpReader implements and RTP receiver. It reads packets from RTP
+// socket, parses them, calculates extended sequence number and then
+// passes them to a callback. It also collects statistics for RTCP
+// receiver reports, but doesn't send any RTCP packets itself.
 class RtpReader : public SocketReaderBase {
  public:
   RtpReader();
@@ -49,11 +61,17 @@ class RtpReader : public SocketReaderBase {
   void Init(net::Socket* socket, OnMessageCallback* on_message_callback);
 
  protected:
+  friend class RtpVideoReaderTest;
+
   virtual void OnDataReceived(net::IOBuffer* buffer, int data_size);
 
  private:
   scoped_ptr<OnMessageCallback> on_message_callback_;
 
+  uint16 max_sequence_number_;
+  uint16 wrap_around_count_;
+  bool started_;
+
   DISALLOW_COPY_AND_ASSIGN(RtpReader);
 };
 
diff --git a/remoting/protocol/rtp_utils.h b/remoting/protocol/rtp_utils.h
index 9049a22..1c2fa4e 100644
--- a/remoting/protocol/rtp_utils.h
+++ b/remoting/protocol/rtp_utils.h
@@ -11,6 +11,17 @@ namespace remoting {
 namespace protocol {
 
 struct RtpHeader {
+  RtpHeader()
+      : padding(false),
+        extension(false),
+        sources(0),
+        marker(false),
+        payload_type(0),
+        sequence_number(0),
+        timestamp(0),
+        sync_source_id(0) {
+  }
+
   // RTP version is always set to 2.
   // version = 2
   bool padding;
@@ -35,6 +46,13 @@ struct Vp8Descriptor {
     LAST_FRAGMENT = 3,
   };
 
+  Vp8Descriptor()
+      : non_reference_frame(false),
+        fragmentation_info(NOT_FRAGMENTED),
+        frame_beginning(false),
+        picture_id(kuint32max) {
+  }
+
   bool non_reference_frame;
   uint8 fragmentation_info;
   bool frame_beginning;
diff --git a/remoting/protocol/rtp_video_reader.cc b/remoting/protocol/rtp_video_reader.cc
index 07f57c4..6a3ddd9 100644
--- a/remoting/protocol/rtp_video_reader.cc
+++ b/remoting/protocol/rtp_video_reader.cc
@@ -47,8 +47,8 @@ void RtpVideoReader::ResetQueue() {
 }
 
 void RtpVideoReader::OnRtpPacket(const RtpPacket* rtp_packet) {
-  uint16 sequence_number = rtp_packet->header().sequence_number;
-  int16 relative_number = sequence_number - last_sequence_number_;
+  uint32 sequence_number = rtp_packet->extended_sequence_number();
+  int32 relative_number = sequence_number - last_sequence_number_;
   int packet_index;
 
   if (packets_queue_.empty()) {
diff --git a/remoting/protocol/rtp_video_reader.h b/remoting/protocol/rtp_video_reader.h
index 412a4f0..6d63714 100644
--- a/remoting/protocol/rtp_video_reader.h
+++ b/remoting/protocol/rtp_video_reader.h
@@ -52,7 +52,7 @@ class RtpVideoReader : public VideoReader {
   RtpReader rtp_reader_;
 
   PacketsQueue packets_queue_;
-  uint16 last_sequence_number_;
+  uint32 last_sequence_number_;
 
   // The stub that processes all received packets.
   VideoStub* video_stub_;
diff --git a/remoting/protocol/rtp_video_reader_unittest.cc b/remoting/protocol/rtp_video_reader_unittest.cc
index 60dba69c..9af3914 100644
--- a/remoting/protocol/rtp_video_reader_unittest.cc
+++ b/remoting/protocol/rtp_video_reader_unittest.cc
@@ -80,19 +80,31 @@ class RtpVideoReaderTest : public testing::Test,
 
   void SplitAndSend(const FragmentInfo fragments[], int count) {
     for (int i = 0; i < count; ++i) {
-      RtpPacket* packet = new RtpPacket();
-
-      packet->mutable_header()->sequence_number = fragments[i].sequence_number;
-      packet->mutable_header()->marker = fragments[i].last;
-      packet->mutable_header()->timestamp = fragments[i].timestamp;
-
-      packet->mutable_vp8_descriptor()->frame_beginning = fragments[i].first;
-      packet->mutable_vp8_descriptor()->fragmentation_info =
-          fragments[i].fragmentation_info;
-      packet->mutable_payload()->AppendCopyOf(
-          &*data_.begin() + fragments[i].start,
-          fragments[i].end - fragments[i].start);
-      reader_->OnRtpPacket(packet);
+      RtpHeader header;
+      header.sequence_number = fragments[i].sequence_number;
+      header.marker = fragments[i].last;
+      header.timestamp = fragments[i].timestamp;
+
+      Vp8Descriptor descriptor;
+      descriptor.non_reference_frame = true;
+      descriptor.frame_beginning = fragments[i].first;
+      descriptor.fragmentation_info = fragments[i].fragmentation_info;
+
+      int header_size = GetRtpHeaderSize(header);
+      int vp8_desc_size = GetVp8DescriptorSize(descriptor);
+      int payload_size = fragments[i].end - fragments[i].start;
+      int size = header_size + vp8_desc_size + payload_size;
+      scoped_refptr<net::IOBuffer> buffer = new net::IOBuffer(size);
+
+      PackRtpHeader(header, reinterpret_cast<uint8*>(buffer->data()),
+                    header_size);
+      PackVp8Descriptor(descriptor, reinterpret_cast<uint8*>(buffer->data()) +
+                        header_size, vp8_desc_size);
+
+      memcpy(buffer->data() + header_size + vp8_desc_size,
+             &*data_.begin() + fragments[i].start, payload_size);
+
+      reader_->rtp_reader_.OnDataReceived(buffer, size);
     }
   }
 
diff --git a/remoting/protocol/rtp_video_writer.cc b/remoting/protocol/rtp_video_writer.cc
index f74d932..da16444 100644
--- a/remoting/protocol/rtp_video_writer.cc
+++ b/remoting/protocol/rtp_video_writer.cc
@@ -23,7 +23,7 @@ RtpVideoWriter::RtpVideoWriter() { }
 RtpVideoWriter::~RtpVideoWriter() { }
 
 void RtpVideoWriter::Init(protocol::Session* session) {
-  rtp_writer_.Init(session->video_rtp_channel(), session->video_rtcp_channel());
+  rtp_writer_.Init(session->video_rtp_channel());
 }
 
 void RtpVideoWriter::ProcessVideoPacket(const VideoPacket* packet, Task* done) {
diff --git a/remoting/protocol/rtp_writer.cc b/remoting/protocol/rtp_writer.cc
index f01576c..4ee700e 100644
--- a/remoting/protocol/rtp_writer.cc
+++ b/remoting/protocol/rtp_writer.cc
@@ -17,20 +17,16 @@ const uint8 kRtpPayloadTypePrivate = 96;
 }  // namespace
 
 RtpWriter::RtpWriter()
-    : rtp_socket_(NULL),
-      rtcp_socket_(NULL),
-      last_packet_number_(0) {
+    : last_packet_number_(0) {
 }
 
 RtpWriter::~RtpWriter() { }
 
 // Initializes the writer. Must be called on the thread the sockets belong
 // to.
-void RtpWriter::Init(net::Socket* rtp_socket, net::Socket* rtcp_socket) {
+void RtpWriter::Init(net::Socket* rtp_socket) {
   buffered_rtp_writer_ = new BufferedDatagramWriter();
   buffered_rtp_writer_->Init(rtp_socket, NULL);
-  rtp_socket_ = rtp_socket;
-  rtcp_socket_ = rtcp_socket;
 }
 
 void RtpWriter::SendPacket(uint32 timestamp, bool marker,
@@ -49,8 +45,7 @@ void RtpWriter::SendPacket(uint32 timestamp, bool marker,
   // so SSRC isn't useful. Implement it in future if neccessary.
   header.sync_source_id = 0;
 
-  // TODO(sergeyu): Handle sequence number wrapping.
-  header.sequence_number = last_packet_number_;
+  header.sequence_number = last_packet_number_ & 0xFFFF;
   ++last_packet_number_;
 
   int header_size = GetRtpHeaderSize(header);
diff --git a/remoting/protocol/rtp_writer.h b/remoting/protocol/rtp_writer.h
index 99bbb49..60c6f0c 100644
--- a/remoting/protocol/rtp_writer.h
+++ b/remoting/protocol/rtp_writer.h
@@ -20,9 +20,9 @@ class RtpWriter {
   RtpWriter();
   virtual ~RtpWriter();
 
-  // Initializes the writer. Must be called on the thread the sockets belong
-  // to.
-  void Init(net::Socket* rtp_socket, net::Socket* rtcp_socket);
+  // Initializes the writer. Must be called on the thread the socket
+  // belongs to.
+  void Init(net::Socket* socket);
 
   // Sends next packet. The packet is mutated by
   void SendPacket(uint32 timestamp, bool marker,
@@ -37,9 +37,6 @@ class RtpWriter {
   void Close();
 
  private:
-  net::Socket* rtp_socket_;
-  net::Socket* rtcp_socket_;
-
   uint32 last_packet_number_;
 
   scoped_refptr<BufferedDatagramWriter> buffered_rtp_writer_;