Fix for making sure that the packet in order checks are done prior to updating the last received packet state.

Without this fix all packets are considered out-of-order by the rtp receiver, causing the last received state
in the rtp receiver to never get valid.

Also makes sure that only valid timestamps and receive times are used for audio/video sync.

BUG=2608
R=mflodman@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/3609004

git-svn-id: http://webrtc.googlecode.com/svn/trunk/webrtc@5102 4adac7df-926f-26a2-2b94-8c16560cd09d

9 files changed, 54 insertions, 31 deletions

diff --git a/modules/rtp_rtcp/interface/rtp_receiver.h b/modules/rtp_rtcp/interface/rtp_receiver.h
index d1f1a818..00e5e5d2 100644
--- a/modules/rtp_rtcp/interface/rtp_receiver.h
+++ b/modules/rtp_rtcp/interface/rtp_receiver.h
@@ -82,10 +82,12 @@ class RtpReceiver {
   // Turn negative acknowledgement (NACK) requests on/off.
   virtual void SetNACKStatus(const NACKMethod method) = 0;
 
-  // Returns the last received timestamp.
-  virtual uint32_t Timestamp() const = 0;
-  // Returns the time in milliseconds when the last timestamp was received.
-  virtual int32_t LastReceivedTimeMs() const = 0;
+  // Gets the last received timestamp. Returns true if a packet has been
+  // received, false otherwise.
+  virtual bool Timestamp(uint32_t* timestamp) const = 0;
+  // Gets the time in milliseconds when the last timestamp was received.
+  // Returns true if a packet has been received, false otherwise.
+  virtual bool LastReceivedTimeMs(int64_t* receive_time_ms) const = 0;
 
   // Returns the remote SSRC of the currently received RTP stream.
   virtual uint32_t SSRC() const = 0;
diff --git a/modules/rtp_rtcp/source/rtp_receiver_impl.cc b/modules/rtp_rtcp/source/rtp_receiver_impl.cc
index 00fa2a48..9a276819 100644
--- a/modules/rtp_rtcp/source/rtp_receiver_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_receiver_impl.cc
@@ -80,7 +80,7 @@ RtpReceiverImpl::RtpReceiverImpl(int32_t id,
       num_csrcs_(0),
       current_remote_csrc_(),
       last_received_timestamp_(0),
-      last_received_frame_time_ms_(0),
+      last_received_frame_time_ms_(-1),
       last_received_sequence_number_(0),
       nack_method_(kNackOff) {
   assert(incoming_audio_messages_callback);
@@ -228,18 +228,20 @@ bool RtpReceiverImpl::IncomingRtpPacket(
   uint16_t payload_data_length = payload_length - rtp_header.paddingLength;
 
   bool is_first_packet_in_frame = false;
-  bool is_first_packet = false;
   {
     CriticalSectionScoped lock(critical_section_rtp_receiver_.get());
-    is_first_packet_in_frame =
+    if (HaveReceivedFrame()) {
+      is_first_packet_in_frame =
           last_received_sequence_number_ + 1 == rtp_header.sequenceNumber &&
-          Timestamp() != rtp_header.timestamp;
-    is_first_packet = is_first_packet_in_frame || last_receive_time_ == 0;
+          last_received_timestamp_ != rtp_header.timestamp;
+    } else {
+      is_first_packet_in_frame = true;
+    }
   }
 
   int32_t ret_val = rtp_media_receiver_->ParseRtpPacket(
       &webrtc_rtp_header, payload_specific, is_red, payload, payload_length,
-      clock_->TimeInMilliseconds(), is_first_packet);
+      clock_->TimeInMilliseconds(), is_first_packet_in_frame);
 
   if (ret_val < 0) {
     return false;
@@ -266,14 +268,24 @@ TelephoneEventHandler* RtpReceiverImpl::GetTelephoneEventHandler() {
   return rtp_media_receiver_->GetTelephoneEventHandler();
 }
 
-uint32_t RtpReceiverImpl::Timestamp() const {
+bool RtpReceiverImpl::Timestamp(uint32_t* timestamp) const {
   CriticalSectionScoped lock(critical_section_rtp_receiver_.get());
-  return last_received_timestamp_;
+  if (!HaveReceivedFrame())
+    return false;
+  *timestamp = last_received_timestamp_;
+  return true;
 }
 
-int32_t RtpReceiverImpl::LastReceivedTimeMs() const {
+bool RtpReceiverImpl::LastReceivedTimeMs(int64_t* receive_time_ms) const {
   CriticalSectionScoped lock(critical_section_rtp_receiver_.get());
-  return last_received_frame_time_ms_;
+  if (!HaveReceivedFrame())
+    return false;
+  *receive_time_ms = last_received_frame_time_ms_;
+  return true;
+}
+
+bool RtpReceiverImpl::HaveReceivedFrame() const {
+  return last_received_frame_time_ms_ >= 0;
 }
 
 // Implementation note: must not hold critsect when called.
@@ -298,7 +310,7 @@ void RtpReceiverImpl::CheckSSRCChanged(const RTPHeader& rtp_header) {
 
       last_received_timestamp_ = 0;
       last_received_sequence_number_ = 0;
-      last_received_frame_time_ms_ = 0;
+      last_received_frame_time_ms_ = -1;
 
       // Do we have a SSRC? Then the stream is restarted.
       if (ssrc_ != 0) {
diff --git a/modules/rtp_rtcp/source/rtp_receiver_impl.h b/modules/rtp_rtcp/source/rtp_receiver_impl.h
index c3a2fa1b..264225cc 100644
--- a/modules/rtp_rtcp/source/rtp_receiver_impl.h
+++ b/modules/rtp_rtcp/source/rtp_receiver_impl.h
@@ -58,8 +58,8 @@ class RtpReceiverImpl : public RtpReceiver {
   void SetNACKStatus(const NACKMethod method);
 
   // Returns the last received timestamp.
-  virtual uint32_t Timestamp() const;
-  int32_t LastReceivedTimeMs() const;
+  bool Timestamp(uint32_t* timestamp) const;
+  bool LastReceivedTimeMs(int64_t* receive_time_ms) const;
 
   uint32_t SSRC() const;
 
@@ -77,6 +77,8 @@ class RtpReceiverImpl : public RtpReceiver {
   TelephoneEventHandler* GetTelephoneEventHandler();
 
  private:
+  bool HaveReceivedFrame() const;
+
   RtpVideoCodecTypes VideoCodecType() const;
 
   void CheckSSRCChanged(const RTPHeader& rtp_header);
diff --git a/modules/rtp_rtcp/test/testAPI/test_api_audio.cc b/modules/rtp_rtcp/test/testAPI/test_api_audio.cc
index 0470def3..78686583 100644
--- a/modules/rtp_rtcp/test/testAPI/test_api_audio.cc
+++ b/modules/rtp_rtcp/test/testAPI/test_api_audio.cc
@@ -232,7 +232,9 @@ TEST_F(RtpRtcpAudioTest, Basic) {
                                          0, -1, test, 4));
 
   EXPECT_EQ(test_ssrc, rtp_receiver2_->SSRC());
-  EXPECT_EQ(test_timestamp, rtp_receiver2_->Timestamp());
+  uint32_t timestamp;
+  EXPECT_TRUE(rtp_receiver2_->Timestamp(&timestamp));
+  EXPECT_EQ(test_timestamp, timestamp);
 }
 
 TEST_F(RtpRtcpAudioTest, RED) {
diff --git a/video_engine/vie_receiver.cc b/video_engine/vie_receiver.cc
index d7ac3c06..0f13aaf8 100644
--- a/video_engine/vie_receiver.cc
+++ b/video_engine/vie_receiver.cc
@@ -260,11 +260,12 @@ int ViEReceiver::InsertRTPPacket(const int8_t* rtp_packet,
                                             payload_length, header);
   header.payload_type_frequency = kVideoPayloadTypeFrequency;
 
+  bool in_order = IsPacketInOrder(header);
   rtp_receive_statistics_->IncomingPacket(header, received_packet_length,
-                                          IsPacketRetransmitted(header));
+      IsPacketRetransmitted(header, in_order));
   rtp_payload_registry_->SetIncomingPayloadType(header);
   return ReceivePacket(received_packet, received_packet_length, header,
-                       IsPacketInOrder(header)) ? 0 : -1;
+                       in_order) ? 0 : -1;
 }
 
 bool ViEReceiver::ReceivePacket(const uint8_t* packet,
@@ -457,7 +458,8 @@ bool ViEReceiver::IsPacketInOrder(const RTPHeader& header) const {
   return statistician->IsPacketInOrder(header.sequenceNumber);
 }
 
-bool ViEReceiver::IsPacketRetransmitted(const RTPHeader& header) const {
+bool ViEReceiver::IsPacketRetransmitted(const RTPHeader& header,
+                                        bool in_order) const {
   // Retransmissions are handled separately if RTX is enabled.
   if (rtp_payload_registry_->RtxEnabled())
     return false;
@@ -468,7 +470,7 @@ bool ViEReceiver::IsPacketRetransmitted(const RTPHeader& header) const {
   // Check if this is a retransmission.
   uint16_t min_rtt = 0;
   rtp_rtcp_->RTT(rtp_receiver_->SSRC(), NULL, NULL, &min_rtt, NULL);
-  return !IsPacketInOrder(header) &&
+  return !in_order &&
       statistician->IsRetransmitOfOldPacket(header, min_rtt);
 }
 }  // namespace webrtc
diff --git a/video_engine/vie_receiver.h b/video_engine/vie_receiver.h
index 0fac8ed6..c71467b8 100644
--- a/video_engine/vie_receiver.h
+++ b/video_engine/vie_receiver.h
@@ -96,7 +96,7 @@ class ViEReceiver : public RtpData {
                                          const RTPHeader& header);
   int InsertRTCPPacket(const int8_t* rtcp_packet, int rtcp_packet_length);
   bool IsPacketInOrder(const RTPHeader& header) const;
-  bool IsPacketRetransmitted(const RTPHeader& header) const;
+  bool IsPacketRetransmitted(const RTPHeader& header, bool in_order) const;
 
   scoped_ptr<CriticalSectionWrapper> receive_cs_;
   const int32_t channel_id_;
diff --git a/video_engine/vie_sync_module.cc b/video_engine/vie_sync_module.cc
index c42fae61..89da022b 100644
--- a/video_engine/vie_sync_module.cc
+++ b/video_engine/vie_sync_module.cc
@@ -26,8 +26,10 @@ enum { kSyncInterval = 1000};
 
 int UpdateMeasurements(StreamSynchronization::Measurements* stream,
                        const RtpRtcp& rtp_rtcp, const RtpReceiver& receiver) {
-  stream->latest_timestamp = receiver.Timestamp();
-  stream->latest_receive_time_ms = receiver.LastReceivedTimeMs();
+  if (!receiver.Timestamp(&stream->latest_timestamp))
+    return -1;
+  if (!receiver.LastReceivedTimeMs(&stream->latest_receive_time_ms))
+    return -1;
   synchronization::RtcpMeasurement measurement;
   if (0 != rtp_rtcp.RemoteNTP(&measurement.ntp_secs,
                               &measurement.ntp_frac,
diff --git a/voice_engine/channel.cc b/voice_engine/channel.cc
index 9e616a35..37016ced 100644
--- a/voice_engine/channel.cc
+++ b/voice_engine/channel.cc
@@ -2107,11 +2107,11 @@ int32_t Channel::ReceivedRTPPacket(const int8_t* data, int32_t length) {
       rtp_payload_registry_->GetPayloadTypeFrequency(header.payloadType);
   if (header.payload_type_frequency < 0)
     return -1;
+  bool in_order = IsPacketInOrder(header);
   rtp_receive_statistics_->IncomingPacket(header, length,
-                                          IsPacketRetransmitted(header));
+      IsPacketRetransmitted(header, in_order));
   rtp_payload_registry_->SetIncomingPayloadType(header);
-  return ReceivePacket(received_packet, length, header,
-                       IsPacketInOrder(header)) ? 0 : -1;
+  return ReceivePacket(received_packet, length, header, in_order) ? 0 : -1;
 }
 
 bool Channel::ReceivePacket(const uint8_t* packet,
@@ -2171,7 +2171,8 @@ bool Channel::IsPacketInOrder(const RTPHeader& header) const {
   return statistician->IsPacketInOrder(header.sequenceNumber);
 }
 
-bool Channel::IsPacketRetransmitted(const RTPHeader& header) const {
+bool Channel::IsPacketRetransmitted(const RTPHeader& header,
+                                    bool in_order) const {
   // Retransmissions are handled separately if RTX is enabled.
   if (rtp_payload_registry_->RtxEnabled())
     return false;
@@ -2182,7 +2183,7 @@ bool Channel::IsPacketRetransmitted(const RTPHeader& header) const {
   // Check if this is a retransmission.
   uint16_t min_rtt = 0;
   _rtpRtcpModule->RTT(rtp_receiver_->SSRC(), NULL, NULL, &min_rtt, NULL);
-  return !IsPacketInOrder(header) &&
+  return !in_order &&
       statistician->IsRetransmitOfOldPacket(header, min_rtt);
 }
 
diff --git a/voice_engine/channel.h b/voice_engine/channel.h
index 39d36a87..4647056b 100644
--- a/voice_engine/channel.h
+++ b/voice_engine/channel.h
@@ -431,7 +431,7 @@ private:
                              int packet_length,
                              const RTPHeader& header);
     bool IsPacketInOrder(const RTPHeader& header) const;
-    bool IsPacketRetransmitted(const RTPHeader& header) const;
+    bool IsPacketRetransmitted(const RTPHeader& header, bool in_order) const;
     int ResendPackets(const uint16_t* sequence_numbers, int length);
     int InsertInbandDtmfTone();
     int32_t MixOrReplaceAudioWithFile(int mixingFrequency);