Cast: Refactor RTCP handling

media/cast/net/rtcp/rtcp_receiver.cc

Index: media/cast/net/rtcp/rtcp_receiver.cc
diff --git a/media/cast/net/rtcp/rtcp_receiver.cc b/media/cast/net/rtcp/rtcp_receiver.cc
index e71d23b8f93bec7238288e7103aaa0fbab753fa2..fd0ad196d60bb12407d12eb0cce896cfa2ece5ea 100644
--- a/media/cast/net/rtcp/rtcp_receiver.cc
+++ b/media/cast/net/rtcp/rtcp_receiver.cc
@@ -4,6 +4,7 @@
 
 #include "media/cast/net/rtcp/rtcp_receiver.h"
 
+#include "base/big_endian.h"
 #include "base/logging.h"
 #include "media/cast/net/cast_transport_defines.h"
 #include "media/cast/net/rtcp/rtcp_utility.h"
@@ -38,21 +39,39 @@ std::pair<uint64, uint64> GetReceiverEventKey(
 namespace media {
 namespace cast {
 
-RtcpReceiver::RtcpReceiver(scoped_refptr<CastEnvironment> cast_environment,
-                           RtcpSenderFeedback* sender_feedback,
-                           RtcpReceiverFeedback* receiver_feedback,
-                           RtcpRttFeedback* rtt_feedback,
+RtcpReceiver::RtcpReceiver(RtcpMessageHandler* handler,
                            uint32 local_ssrc)
     : ssrc_(local_ssrc),
       remote_ssrc_(0),
-      sender_feedback_(sender_feedback),
-      receiver_feedback_(receiver_feedback),
-      rtt_feedback_(rtt_feedback),
-      cast_environment_(cast_environment),
+      handler_(handler),
       receiver_event_history_size_(0) {}

[miu, 2014/07/16 00:09:32]

To simplify, consider adding DCHECK(handler_) here, and then remove all the NULL
checks in the rest of this module.

[Alpha Left Google, 2014/07/17 01:01:45]

Done.

 
 RtcpReceiver::~RtcpReceiver() {}
 
+// static
+bool RtcpReceiver::IsRtcpPacket(const uint8* packet, size_t length) {

[miu, 2014/07/16 00:09:32]

You could get rid of this method, and roll it into GetSsrcOfSender.  See
comments below.

[Alpha Left Google, 2014/07/17 01:01:45]

0 is a valid SSRC so I would like to keep this method.

+  DCHECK_GE(length, kMinLengthOfRtcp) << "Invalid RTCP packet";
+  if (length < kMinLengthOfRtcp) return false;
+
+  uint8 packet_type = packet[1];
+  if (packet_type >= kPacketTypeLow &&
+      packet_type <= kPacketTypeHigh) {
+    return true;
+  }
+  return false;
+}
+
+// static
+uint32 RtcpReceiver::GetSsrcOfSender(const uint8* rtcp_buffer, size_t length) {
+  DCHECK_GE(length, kMinLengthOfRtcp) << "Invalid RTCP packet";

[miu, 2014/07/16 00:09:32]

Don't need this DCHECK if checking big_endian_reader.XXX() return values (see
below).

+  uint32 ssrc_of_sender;
+  base::BigEndianReader big_endian_reader(
+      reinterpret_cast<const char*>(rtcp_buffer), length);
+  big_endian_reader.Skip(4);  // Skip header

[miu, 2014/07/16 00:09:32]

Note: These two method calls on big_endian_reader return false on failure (i.e.,
they check bounds).  Suggest replacing with this code:

  uint8 packet_type;
  if (big_endian_reader.Skip(2) &&
      big_endian_reader.ReadU8(&packet_type) &&
      packet_type >= kPacketTypeLow &&
      packet_type <= kPacketTypeHigh &&
      big_endian_reader.Skip(1) &&
      big_endian_reader.ReadU32(&ssrc_of_sender)) {
    return ssrc_of_sender;
  } else {
    return 0;
  }

[Alpha Left Google, 2014/07/17 01:01:45]

IsRtcpPacket is actually useful because 0 is also a valid SSRC. The bounds check
is guaranteed by kMinLengthOfRtcp already, I've made it an if statement.

+  big_endian_reader.ReadU32(&ssrc_of_sender);
+  return ssrc_of_sender;
+}
+
 void RtcpReceiver::SetRemoteSSRC(uint32 ssrc) { remote_ssrc_ = ssrc; }
 
 void RtcpReceiver::SetCastReceiverEventHistorySize(size_t size) {
@@ -83,9 +102,6 @@ void RtcpReceiver::IncomingRtcpPacket(RtcpParser* rtcp_parser) {
       case kRtcpGenericRtpFeedbackNackCode:
         HandleNACK(rtcp_parser);
         break;
-      case kRtcpGenericRtpFeedbackSrReqCode:
-        HandleSendReportRequest(rtcp_parser);
-        break;
       case kRtcpPayloadSpecificPliCode:
         HandlePLI(rtcp_parser);
         break;
@@ -110,6 +126,7 @@ void RtcpReceiver::IncomingRtcpPacket(RtcpParser* rtcp_parser) {
       case kRtcpNotValidCode:
       case kRtcpReportBlockItemCode:
       case kRtcpSdesChunkCode:
+      case kRtcpGenericRtpFeedbackSrReqCode:
       case kRtcpGenericRtpFeedbackNackItemCode:
       case kRtcpPayloadSpecificFirItemCode:
       case kRtcpXrRrtrCode:
@@ -145,8 +162,8 @@ void RtcpReceiver::HandleSenderReport(RtcpParser* rtcp_parser) {
         rtcp_field.sender_report.sender_packet_count;
     remote_sender_info.send_octet_count =
         rtcp_field.sender_report.sender_octet_count;
-    if (receiver_feedback_) {
-      receiver_feedback_->OnReceivedSenderReport(remote_sender_info);
+    if (handler_) {
+      handler_->OnReceivedSenderReport(remote_sender_info);
     }
   }
   rtcp_field_type = rtcp_parser->Iterate();
@@ -204,9 +221,9 @@ void RtcpReceiver::HandleReportBlock(const RtcpField* rtcp_field,
   report_block.last_sr = rb.last_sender_report;
   report_block.delay_since_last_sr = rb.delay_last_sender_report;
 
-  if (rtt_feedback_) {
-    rtt_feedback_->OnReceivedDelaySinceLastReport(
-        rb.ssrc, rb.last_sender_report, rb.delay_last_sender_report);
+  if (handler_) {
+    handler_->OnReceivedDelaySinceLastReport(
+        rb.last_sender_report, rb.delay_last_sender_report);
   }
 }
 
@@ -255,8 +272,8 @@ void RtcpReceiver::HandleRrtr(RtcpParser* rtcp_parser, uint32 remote_ssrc) {
   remote_time_report.ntp_seconds = rtcp_field.rrtr.ntp_most_significant;
   remote_time_report.ntp_fraction = rtcp_field.rrtr.ntp_least_significant;
 
-  if (receiver_feedback_) {
-    receiver_feedback_->OnReceiverReferenceTimeReport(remote_time_report);
+  if (handler_) {
+    handler_->OnReceiverReferenceTimeReport(remote_time_report);
   }
 }
 
@@ -266,9 +283,8 @@ void RtcpReceiver::HandleDlrr(RtcpParser* rtcp_parser) {
     // Not to us.
     return;
   }
-  if (rtt_feedback_) {
-    rtt_feedback_->OnReceivedDelaySinceLastReport(
-        rtcp_field.dlrr.receivers_ssrc,
+  if (handler_) {
+    handler_->OnReceivedDelaySinceLastReport(
         rtcp_field.dlrr.last_receiver_report,
         rtcp_field.dlrr.delay_last_receiver_report);
   }
@@ -326,13 +342,6 @@ void RtcpReceiver::HandlePLI(RtcpParser* rtcp_parser) {
   rtcp_parser->Iterate();
 }
 
-void RtcpReceiver::HandleSendReportRequest(RtcpParser* rtcp_parser) {
-  if (receiver_feedback_) {
-    receiver_feedback_->OnReceivedSendReportRequest();
-  }
-  rtcp_parser->Iterate();
-}
-
 void RtcpReceiver::HandleRpsi(RtcpParser* rtcp_parser) {
   const RtcpField& rtcp_field = rtcp_parser->Field();
   if (rtcp_parser->Iterate() != kRtcpPayloadSpecificRpsiCode) {
@@ -436,8 +445,8 @@ void RtcpReceiver::HandleApplicationSpecificCastReceiverLog(
       receiver_log.push_back(frame_log);
   }
 
-  if (receiver_feedback_ && !receiver_log.empty()) {
-    receiver_feedback_->OnReceivedReceiverLog(receiver_log);
+  if (handler_ && !receiver_log.empty()) {
+    handler_->OnReceivedReceiverLog(receiver_log);
   }
 }
 
@@ -502,8 +511,8 @@ void RtcpReceiver::HandlePayloadSpecificCastItem(RtcpParser* rtcp_parser) {
         &rtcp_field, &cast_message.missing_frames_and_packets_);
     packet_type = rtcp_parser->Iterate();
   }
-  if (sender_feedback_) {
-    sender_feedback_->OnReceivedCastFeedback(cast_message);
+  if (handler_) {
+    handler_->OnReceivedCastFeedback(cast_message);
   }
 }