Cast: Fix rtcp event dedup logic in rtcp_receiver.

media/cast/rtcp/rtcp_receiver.cc

 // Copyright 2013 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 "media/cast/rtcp/rtcp_receiver.h"
 
 #include "base/logging.h"
 #include "media/cast/rtcp/rtcp_utility.h"
 #include "media/cast/transport/cast_transport_defines.h"
 
 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
 TranslateToFrameStatusFromWireFormat(uint8 status) {
   switch (status) {
     case 0:
       return media::cast::transport::kRtcpSenderFrameStatusUnknown;
     case 1:
       return media::cast::transport::kRtcpSenderFrameStatusDroppedByEncoder;
     case 2:
       return media::cast::transport::kRtcpSenderFrameStatusDroppedByFlowControl;
     case 3:
       return media::cast::transport::kRtcpSenderFrameStatusSentToNetwork;
     default:
       // If the sender adds new log messages we will end up here until we add
       // the new messages in the receiver.
       NOTREACHED();
       VLOG(1) << "Unexpected status received: " << static_cast<int>(status);
       return media::cast::transport::kRtcpSenderFrameStatusUnknown;
   }
 }
 
-// A receiver event is identified by frame RTP timestamp, event timestamp and
-// 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.
 size_t HashReceiverEvent(uint32 frame_rtp_timestamp,

[Alpha Left Google, 2014/05/06 20:55:52]

I suggest we change the hash set to use a key of std::pair<uint64, uint64>. I
thought size_t was enough. This means the return type of this method should be:

typedef std::pair<uint64, uint64> ReceiverEventKey;

// <Describe how to combine multiple fields here..>
ReceiverEventKey ComputeReceiverEventKey(...);

[imcheng, 2014/05/06 21:19:47]

Are you suggesting that we get rid of the base::HashInts64 call below and just
let the hash_set take care of the hashing? Because reading this again, I do not
understand why is it necessary to call base::HashInts64.

On 2014/05/06 20:55:52, Alpha wrote:

[Alpha Left Google, 2014/05/06 21:21:45]

That's right. I used it because base::hash_map might not support std::pair<> on
some platform and size_t was easier. But for correctness (that this patch is
trying to fix) using std::pair<> is the right thing to do.

[imcheng, 2014/05/06 22:29:39]

Done.

                          const base::TimeTicks& event_timestamp,
-                         media::cast::CastLoggingEvent event_type) {
+                         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(
       value1, static_cast<uint64>(event_timestamp.ToInternalValue()));
 }
 
 }  // namespace
 
 namespace media {
 namespace cast {
@@ skipping 403 matching lines @@
     receiver_feedback_->OnReceivedReceiverLog(receiver_log);
   }
 }
 
 void RtcpReceiver::HandleApplicationSpecificCastReceiverEventLog(
     uint32 frame_rtp_timestamp,
     RtcpParser* rtcp_parser,
     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(
           rtcp_field.cast_receiver_log.event_timestamp_base +
           rtcp_field.cast_receiver_log.event_timestamp_delta);
 
   // The following code checks to see if we have already seen this event.
   // The algorithm works by maintaining a sliding window of events. We have
   // 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);
+      HashReceiverEvent(frame_rtp_timestamp, event_timestamp, event, packet_id);
   if (receiver_event_hash_set_.find(event_hash) !=
       receiver_event_hash_set_.end()) {
     return;
   } else {
     receiver_event_hash_set_.insert(event_hash);
     receiver_event_hash_queue_.push(event_hash);
 
     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();
@@ skipping 108 matching lines @@
 void RtcpReceiver::HandleFIRItem(const RtcpField* rtcp_field) {
   // Is it our sender that is requested to generate a new keyframe.
   if (ssrc_ != rtcp_field->fir_item.ssrc)
     return;
 
   VLOG(2) << "Cast RTCP received FIR on our SSRC " << ssrc_;
 }
 
 }  // namespace cast
 }  // namespace media