Cast: Fix rtcp event dedup logic in rtcp_receiver.

Previously the dedup logic is based on a hash of rtp timestamp, event
time, and event type. This is not sufficient for packet events since
two of them can have the same fields except for packet id. The hash
needs to also take account into packet id for packet events.

Review URL: https://codereview.chromium.org/266373008

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

9 files changed, 135 insertions, 98 deletions

diff --git a/media/cast/rtcp/rtcp_receiver.cc b/media/cast/rtcp/rtcp_receiver.cc
index 8c0e593..9345d58 100644
--- a/media/cast/rtcp/rtcp_receiver.cc
+++ b/media/cast/rtcp/rtcp_receiver.cc
@@ -10,35 +10,11 @@
 
 namespace {
 
-media::cast::CastLoggingEvent TranslateToLogEventFromWireFormat(uint8 event) {
-  switch (event) {
-    case 1:
-      return media::cast::kAudioAckSent;
-    case 2:
-      return media::cast::kAudioPlayoutDelay;
-    case 3:
-      return media::cast::kAudioFrameDecoded;
-    case 4:
-      return media::cast::kAudioPacketReceived;
-    case 5:
-      return media::cast::kVideoAckSent;
-    case 6:
-      return media::cast::kVideoFrameDecoded;
-    case 7:
-      return media::cast::kVideoRenderDelay;
-    case 8:
-      return media::cast::kVideoPacketReceived;
-    case 9:
-      return media::cast::kDuplicateAudioPacketReceived;
-    case 10:
-      return media::cast::kDuplicateVideoPacketReceived;
-    default:
-      // If the sender adds new log messages we will end up here until we add
-      // the new messages in the receiver.
-      VLOG(1) << "Unexpected log message received: " << static_cast<int>(event);
-      NOTREACHED();
-      return media::cast::kUnknown;
-  }
+bool IsRtcpPacketEvent(media::cast::CastLoggingEvent event_type) {
+  return event_type == media::cast::kAudioPacketReceived ||
+         event_type == media::cast::kVideoPacketReceived ||
+         event_type == media::cast::kDuplicateAudioPacketReceived ||
+         event_type == media::cast::kDuplicateVideoPacketReceived;
 }
 
 media::cast::transport::RtcpSenderFrameStatus
@@ -61,15 +37,26 @@ TranslateToFrameStatusFromWireFormat(uint8 status) {
   }
 }
 
-// A receiver event is identified by frame RTP timestamp, event timestamp and
-// event type.
-size_t HashReceiverEvent(uint32 frame_rtp_timestamp,
-                         const base::TimeTicks& event_timestamp,
-                         media::cast::CastLoggingEvent event_type) {
+// A receiver frame event is identified by frame RTP timestamp, event timestamp
+// and event type.
+// A receiver packet event is identified by all of the above plus packet id.
+// The key format is as follows:
+// First uint64:
+//   bits 0-11: zeroes (unused).
+//   bits 12-15: event type ID.
+//   bits 16-31: packet ID if packet event, 0 otherwise.
+//   bits 32-63: RTP timestamp.
+// Second uint64:
+//   bits 0-63: event TimeTicks internal value.
+std::pair<uint64, uint64> GetReceiverEventKey(
+    uint32 frame_rtp_timestamp, const base::TimeTicks& event_timestamp,
+    uint8 event_type, uint16 packet_id_or_zero) {
   uint64 value1 = event_type;
+  value1 <<= 16;
+  value1 |= packet_id_or_zero;
   value1 <<= 32;
   value1 |= frame_rtp_timestamp;
-  return base::HashInts64(
+  return std::make_pair(
       value1, static_cast<uint64>(event_timestamp.ToInternalValue()));
 }
 
@@ -490,8 +477,10 @@ void RtcpReceiver::HandleApplicationSpecificCastReceiverEventLog(
     RtcpReceiverEventLogMessages* event_log_messages) {
   const RtcpField& rtcp_field = rtcp_parser->Field();
 
-  const CastLoggingEvent event_type =
-      TranslateToLogEventFromWireFormat(rtcp_field.cast_receiver_log.event);
+  const uint8 event = rtcp_field.cast_receiver_log.event;
+  const CastLoggingEvent event_type = TranslateToLogEventFromWireFormat(event);
+  uint16 packet_id = IsRtcpPacketEvent(event_type) ?
+      rtcp_field.cast_receiver_log.delay_delta_or_packet_id.packet_id : 0;
   const base::TimeTicks event_timestamp =
       base::TimeTicks() +
       base::TimeDelta::FromMilliseconds(
@@ -503,21 +492,19 @@ void RtcpReceiver::HandleApplicationSpecificCastReceiverEventLog(
   // a queue and a set of events. We enqueue every new event and insert it
   // into the set. When the queue becomes too big we remove the oldest event
   // from both the queue and the set.
-  // Different events may have the same hash value. That's okay because full
-  // accuracy is not important in this case.
-  const size_t event_hash =
-      HashReceiverEvent(frame_rtp_timestamp, event_timestamp, event_type);
-  if (receiver_event_hash_set_.find(event_hash) !=
-      receiver_event_hash_set_.end()) {
+  ReceiverEventKey key =
+      GetReceiverEventKey(
+          frame_rtp_timestamp, event_timestamp, event, packet_id);
+  if (receiver_event_key_set_.find(key) != receiver_event_key_set_.end()) {
     return;
   } else {
-    receiver_event_hash_set_.insert(event_hash);
-    receiver_event_hash_queue_.push(event_hash);
+    receiver_event_key_set_.insert(key);
+    receiver_event_key_queue_.push(key);
 
-    if (receiver_event_hash_queue_.size() > receiver_event_history_size_) {
-      const size_t oldest_hash = receiver_event_hash_queue_.front();
-      receiver_event_hash_queue_.pop();
-      receiver_event_hash_set_.erase(oldest_hash);
+    if (receiver_event_key_queue_.size() > receiver_event_history_size_) {
+      const ReceiverEventKey oldest_key = receiver_event_key_queue_.front();
+      receiver_event_key_queue_.pop();
+      receiver_event_key_set_.erase(oldest_key);
     }
   }
 
diff --git a/media/cast/rtcp/rtcp_receiver.h b/media/cast/rtcp/rtcp_receiver.h
index 2e88246..c8b6435 100644
--- a/media/cast/rtcp/rtcp_receiver.h
+++ b/media/cast/rtcp/rtcp_receiver.h
@@ -123,8 +123,9 @@ class RtcpReceiver {
 
   // Maintains a history of receiver events.
   size_t receiver_event_history_size_;
-  base::hash_set<size_t> receiver_event_hash_set_;
-  std::queue<size_t> receiver_event_hash_queue_;
+  typedef std::pair<uint64, uint64> ReceiverEventKey;
+  base::hash_set<ReceiverEventKey> receiver_event_key_set_;
+  std::queue<ReceiverEventKey> receiver_event_key_queue_;
 
   DISALLOW_COPY_AND_ASSIGN(RtcpReceiver);
 };
diff --git a/media/cast/rtcp/rtcp_receiver_unittest.cc b/media/cast/rtcp/rtcp_receiver_unittest.cc
index 8e8e4f9..74a60dc 100644
--- a/media/cast/rtcp/rtcp_receiver_unittest.cc
+++ b/media/cast/rtcp/rtcp_receiver_unittest.cc
@@ -511,6 +511,12 @@ TEST_F(RtcpReceiverTest, InjectReceiverReportWithReceiverLogVerificationBase) {
   event_log.event_timestamp = testing_clock.NowTicks();
   event_log.packet_id = kLostPacketId1;
   frame_log.event_log_messages_.push_back(event_log);
+
+  event_log.type = kVideoPacketReceived;
+  event_log.event_timestamp = testing_clock.NowTicks();
+  event_log.packet_id = kLostPacketId2;
+  frame_log.event_log_messages_.push_back(event_log);
+
   receiver_log.push_back(frame_log);
 
   cast_log_verification.SetExpectedReceiverLog(receiver_log);
@@ -519,14 +525,16 @@ TEST_F(RtcpReceiverTest, InjectReceiverReportWithReceiverLogVerificationBase) {
   p.AddRr(kSenderSsrc, 1);
   p.AddRb(kSourceSsrc);
   p.AddReceiverLog(kSenderSsrc);
-  p.AddReceiverFrameLog(kRtpTimestamp, 2, kTimeBaseMs);
-  p.AddReceiverEventLog(kDelayDeltaMs, 5, 0);
-  p.AddReceiverEventLog(kLostPacketId1, 8, kTimeDelayMs);
+  p.AddReceiverFrameLog(kRtpTimestamp, 3, kTimeBaseMs);
+  p.AddReceiverEventLog(kDelayDeltaMs, kVideoAckSent, 0);
+  p.AddReceiverEventLog(kLostPacketId1, kVideoPacketReceived, kTimeDelayMs);
+  p.AddReceiverEventLog(kLostPacketId2, kVideoPacketReceived, kTimeDelayMs);
 
   // Adds duplicated receiver event.
-  p.AddReceiverFrameLog(kRtpTimestamp, 2, kTimeBaseMs);
-  p.AddReceiverEventLog(kDelayDeltaMs, 5, 0);
-  p.AddReceiverEventLog(kLostPacketId1, 8, kTimeDelayMs);
+  p.AddReceiverFrameLog(kRtpTimestamp, 3, kTimeBaseMs);
+  p.AddReceiverEventLog(kDelayDeltaMs, kVideoAckSent, 0);
+  p.AddReceiverEventLog(kLostPacketId1, kVideoPacketReceived, kTimeDelayMs);
+  p.AddReceiverEventLog(kLostPacketId2, kVideoPacketReceived, kTimeDelayMs);
 
   EXPECT_CALL(mock_rtt_feedback_,
               OnReceivedDelaySinceLastReport(
@@ -574,7 +582,7 @@ TEST_F(RtcpReceiverTest, InjectReceiverReportWithReceiverLogVerificationMulti) {
   p.AddReceiverLog(kSenderSsrc);
   for (int i = 0; i < 100; ++i) {
     p.AddReceiverFrameLog(kRtpTimestamp, 1, kTimeBaseMs + i * kTimeDelayMs);
-    p.AddReceiverEventLog(kDelayDeltaMs, 5, 0);
+    p.AddReceiverEventLog(kDelayDeltaMs, kVideoAckSent, 0);
   }
 
   EXPECT_CALL(mock_rtt_feedback_,
diff --git a/media/cast/rtcp/rtcp_sender.cc b/media/cast/rtcp/rtcp_sender.cc
index a95e1de..ba1a079 100644
--- a/media/cast/rtcp/rtcp_sender.cc
+++ b/media/cast/rtcp/rtcp_sender.cc
@@ -25,36 +25,6 @@ namespace {
 // 12 bits.
 const int64 kMaxWireFormatTimeDeltaMs = INT64_C(0xfff);
 
-// Converts a log event type to an integer value.
-// NOTE: We have only allocated 4 bits to represent the type of event over the
-// wire. Therefore, this function can only return values from 0 to 15.
-int ConvertEventTypeToWireFormat(const CastLoggingEvent& event) {
-  switch (event) {
-    case kAudioAckSent:
-      return 1;
-    case kAudioPlayoutDelay:
-      return 2;
-    case kAudioFrameDecoded:
-      return 3;
-    case kAudioPacketReceived:
-      return 4;
-    case kVideoAckSent:
-      return 5;
-    case kVideoFrameDecoded:
-      return 6;
-    case kVideoRenderDelay:
-      return 7;
-    case kVideoPacketReceived:
-      return 8;
-    case kDuplicateAudioPacketReceived:
-      return 9;
-    case kDuplicateVideoPacketReceived:
-      return 10;
-    default:
-      return 0;  // Not an interesting event.
-  }
-}
-
 uint16 MergeEventTypeAndTimestampForWireFormat(
     const CastLoggingEvent& event,
     const base::TimeDelta& time_delta) {
diff --git a/media/cast/rtcp/rtcp_sender_unittest.cc b/media/cast/rtcp/rtcp_sender_unittest.cc
index 0d0a6868..6746a68 100644
--- a/media/cast/rtcp/rtcp_sender_unittest.cc
+++ b/media/cast/rtcp/rtcp_sender_unittest.cc
@@ -273,8 +273,8 @@ TEST_F(RtcpSenderTest, RtcpReceiverReportWithRrtrCastMessageAndLog) {
 
   p.AddReceiverLog(kSendingSsrc);
   p.AddReceiverFrameLog(kRtpTimestamp, 2, kTimeBaseMs);
-  p.AddReceiverEventLog(0, 5, 0);
-  p.AddReceiverEventLog(kLostPacketId1, 8, kTimeDelayMs);
+  p.AddReceiverEventLog(0, kVideoAckSent, 0);
+  p.AddReceiverEventLog(kLostPacketId1, kVideoPacketReceived, kTimeDelayMs);
 
   test_transport_.SetExpectedRtcpPacket(p.GetPacket().Pass());
 
@@ -335,7 +335,8 @@ TEST_F(RtcpSenderTest, RtcpReceiverReportWithOversizedFrameLog) {
       kTimeBaseMs + (kRtcpMaxReceiverLogMessages - num_events) * kTimeDelayMs);
   for (int i = 0; i < num_events; i++) {
     p.AddReceiverEventLog(
-        kLostPacketId1, 8, static_cast<uint16>(kTimeDelayMs * i));
+        kLostPacketId1, kVideoPacketReceived,
+        static_cast<uint16>(kTimeDelayMs * i));
   }
 
   test_transport_.SetExpectedRtcpPacket(p.GetPacket().Pass());
@@ -399,7 +400,7 @@ TEST_F(RtcpSenderTest, RtcpReceiverReportWithTooManyLogFrames) {
        i < kRtcpMaxReceiverLogMessages;
        ++i) {
     p.AddReceiverFrameLog(kRtpTimestamp + i, 1, kTimeBaseMs + i * kTimeDelayMs);
-    p.AddReceiverEventLog(0, 5, 0);
+    p.AddReceiverEventLog(0, kVideoAckSent, 0);
   }
   test_transport_.SetExpectedRtcpPacket(p.GetPacket().Pass());
 
@@ -450,7 +451,7 @@ TEST_F(RtcpSenderTest, RtcpReceiverReportWithOldLogFrames) {
   const int kTimeBetweenEventsMs = 410;
   p.AddReceiverFrameLog(kRtpTimestamp, 10, kTimeBaseMs + kTimeBetweenEventsMs);
   for (int i = 0; i < 10; ++i) {
-    p.AddReceiverEventLog(0, 5, i * kTimeBetweenEventsMs);
+    p.AddReceiverEventLog(0, kVideoAckSent, i * kTimeBetweenEventsMs);
   }
   test_transport_.SetExpectedRtcpPacket(p.GetPacket().Pass());
 
@@ -505,17 +506,17 @@ TEST_F(RtcpSenderTest, RtcpReceiverReportRedundancy) {
           kRtpTimestamp,
           1,
           time_base_ms - kSecondRedundancyOffset * kTimeBetweenEventsMs);
-      p.AddReceiverEventLog(0, 5, 0);
+      p.AddReceiverEventLog(0, kVideoAckSent, 0);
     }
     if (i >= kFirstRedundancyOffset) {
       p.AddReceiverFrameLog(
           kRtpTimestamp,
           1,
           time_base_ms - kFirstRedundancyOffset * kTimeBetweenEventsMs);
-      p.AddReceiverEventLog(0, 5, 0);
+      p.AddReceiverEventLog(0, kVideoAckSent, 0);
     }
     p.AddReceiverFrameLog(kRtpTimestamp, 1, time_base_ms);
-    p.AddReceiverEventLog(0, 5, 0);
+    p.AddReceiverEventLog(0, kVideoAckSent, 0);
 
     test_transport_.SetExpectedRtcpPacket(p.GetPacket().Pass());
 
diff --git a/media/cast/rtcp/rtcp_utility.cc b/media/cast/rtcp/rtcp_utility.cc
index fd86e8e..da6ef9d 100644
--- a/media/cast/rtcp/rtcp_utility.cc
+++ b/media/cast/rtcp/rtcp_utility.cc
@@ -1051,5 +1051,63 @@ bool RtcpParser::ParseExtendedReportDelaySinceLastReceiverReport() {
   return true;
 }
 
+uint8 ConvertEventTypeToWireFormat(CastLoggingEvent event) {
+  switch (event) {
+    case kAudioAckSent:
+      return 1;
+    case kAudioPlayoutDelay:
+      return 2;
+    case kAudioFrameDecoded:
+      return 3;
+    case kAudioPacketReceived:
+      return 4;
+    case kVideoAckSent:
+      return 5;
+    case kVideoFrameDecoded:
+      return 6;
+    case kVideoRenderDelay:
+      return 7;
+    case kVideoPacketReceived:
+      return 8;
+    case kDuplicateAudioPacketReceived:
+      return 9;
+    case kDuplicateVideoPacketReceived:
+      return 10;
+    default:
+      return 0;  // Not an interesting event.
+  }
+}
+
+CastLoggingEvent TranslateToLogEventFromWireFormat(uint8 event) {
+  switch (event) {
+    case 1:
+      return media::cast::kAudioAckSent;
+    case 2:
+      return media::cast::kAudioPlayoutDelay;
+    case 3:
+      return media::cast::kAudioFrameDecoded;
+    case 4:
+      return media::cast::kAudioPacketReceived;
+    case 5:
+      return media::cast::kVideoAckSent;
+    case 6:
+      return media::cast::kVideoFrameDecoded;
+    case 7:
+      return media::cast::kVideoRenderDelay;
+    case 8:
+      return media::cast::kVideoPacketReceived;
+    case 9:
+      return media::cast::kDuplicateAudioPacketReceived;
+    case 10:
+      return media::cast::kDuplicateVideoPacketReceived;
+    default:
+      // If the sender adds new log messages we will end up here until we add
+      // the new messages in the receiver.
+      VLOG(1) << "Unexpected log message received: " << static_cast<int>(event);
+      NOTREACHED();
+      return media::cast::kUnknown;
+  }
+}
+
 }  // namespace cast
 }  // namespace media
diff --git a/media/cast/rtcp/rtcp_utility.h b/media/cast/rtcp/rtcp_utility.h
index 8d6d00e..fa85744 100644
--- a/media/cast/rtcp/rtcp_utility.h
+++ b/media/cast/rtcp/rtcp_utility.h
@@ -7,6 +7,7 @@
 
 #include "media/cast/cast_config.h"
 #include "media/cast/cast_defines.h"
+#include "media/cast/logging/logging_defines.h"
 #include "media/cast/rtcp/rtcp_defines.h"
 
 namespace media {
@@ -343,6 +344,14 @@ class RtcpParser {
   DISALLOW_COPY_AND_ASSIGN(RtcpParser);
 };
 
+// Converts a log event type to an integer value.
+// NOTE: We have only allocated 4 bits to represent the type of event over the
+// wire. Therefore, this function can only return values from 0 to 15.
+uint8 ConvertEventTypeToWireFormat(CastLoggingEvent event);
+
+// The inverse of |ConvertEventTypeToWireFormat()|.
+CastLoggingEvent TranslateToLogEventFromWireFormat(uint8 event);
+
 }  // namespace cast
 }  // namespace media
 
diff --git a/media/cast/rtcp/test_rtcp_packet_builder.cc b/media/cast/rtcp/test_rtcp_packet_builder.cc
index 8688bd6..a7c5e2c 100644
--- a/media/cast/rtcp/test_rtcp_packet_builder.cc
+++ b/media/cast/rtcp/test_rtcp_packet_builder.cc
@@ -3,7 +3,9 @@
 // found in the LICENSE file.
 
 #include "media/cast/rtcp/test_rtcp_packet_builder.h"
+
 #include "base/logging.h"
+#include "media/cast/rtcp/rtcp_utility.h"
 
 namespace media {
 namespace cast {
@@ -246,9 +248,10 @@ void TestRtcpPacketBuilder::AddReceiverFrameLog(uint32 rtp_timestamp,
 }
 
 void TestRtcpPacketBuilder::AddReceiverEventLog(uint16 event_data,
-                                                uint8 event_id,
+                                                CastLoggingEvent event,
                                                 uint16 event_timesamp_delta) {
   big_endian_writer_.WriteU16(event_data);
+  uint8 event_id = ConvertEventTypeToWireFormat(event);
   uint16 type_and_delta = static_cast<uint16>(event_id) << 12;
   type_and_delta += event_timesamp_delta & 0x0fff;
   big_endian_writer_.WriteU16(type_and_delta);
diff --git a/media/cast/rtcp/test_rtcp_packet_builder.h b/media/cast/rtcp/test_rtcp_packet_builder.h
index 7aa0fd5..eb4a2b7 100644
--- a/media/cast/rtcp/test_rtcp_packet_builder.h
+++ b/media/cast/rtcp/test_rtcp_packet_builder.h
@@ -87,7 +87,7 @@ class TestRtcpPacketBuilder {
                            int num_events,
                            uint32 event_timesamp_base);
   void AddReceiverEventLog(uint16 event_data,
-                           uint8 event_id,
+                           CastLoggingEvent event,
                            uint16 event_timesamp_delta);
 
   scoped_ptr<Packet> GetPacket();