Cast: Refactor RTCP handling

media/cast/net/cast_transport_sender_impl.cc

Index: media/cast/net/cast_transport_sender_impl.cc
diff --git a/media/cast/net/cast_transport_sender_impl.cc b/media/cast/net/cast_transport_sender_impl.cc
index 1e3f71845881ed1d4f59614ee4e83747050f68a5..9146d49124315bb7eaf334398e630aa0bf668f0a 100644
--- a/media/cast/net/cast_transport_sender_impl.cc
+++ b/media/cast/net/cast_transport_sender_impl.cc
@@ -44,19 +44,19 @@ CastTransportSenderImpl::CastTransportSenderImpl(
     : clock_(clock),
       status_callback_(status_callback),
       transport_task_runner_(transport_task_runner),
+      logging_(),

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

nit: You can just remove this.

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

Done.

       transport_(external_transport ? NULL
                                     : new UdpTransport(net_log,
                                                        transport_task_runner,
                                                        net::IPEndPoint(),
                                                        remote_end_point,
                                                        status_callback)),
-      logging_(),
       pacer_(clock,
              &logging_,
              external_transport ? external_transport : transport_.get(),
              transport_task_runner),
-      rtcp_builder_(&pacer_),
-      raw_events_callback_(raw_events_callback) {
+      raw_events_callback_(raw_events_callback),
+      weak_factory_(this) {
   DCHECK(clock_);
   if (!raw_events_callback_.is_null()) {
     DCHECK(raw_events_callback_interval > base::TimeDelta());
@@ -80,7 +80,9 @@ CastTransportSenderImpl::~CastTransportSenderImpl() {
 }
 
 void CastTransportSenderImpl::InitializeAudio(
-    const CastTransportRtpConfig& config) {
+    const CastTransportRtpConfig& config,
+    const RtcpCastMessageCallback& cast_message_cb,
+    const RtcpRttCallback& rtt_cb) {
   LOG_IF(WARNING, config.aes_key.empty() || config.aes_iv_mask.empty())
       << "Unsafe to send audio with encryption DISABLED.";
   if (!audio_encryptor_.Initialize(config.aes_key, config.aes_iv_mask)) {
@@ -95,10 +97,25 @@ void CastTransportSenderImpl::InitializeAudio(
     audio_sender_.reset();
     status_callback_.Run(TRANSPORT_AUDIO_UNINITIALIZED);

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

Need a return statement after this statement.

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

Done.

   }
+  audio_rtcp_session_.reset(
+      new Rtcp(cast_message_cb,
+               rtt_cb,
+               base::Bind(&CastTransportSenderImpl::OnReceivedLogMessage,
+                          weak_factory_.GetWeakPtr(), AUDIO_EVENT),
+               clock_,
+               &pacer_,
+               config.ssrc,
+               config.feedback_ssrc,
+               config.c_name));
+  // Only start receiving once.
+  if (!video_sender_)
+    StartReceiving();
 }
 
 void CastTransportSenderImpl::InitializeVideo(
-    const CastTransportRtpConfig& config) {
+    const CastTransportRtpConfig& config,
+    const RtcpCastMessageCallback& cast_message_cb,
+    const RtcpRttCallback& rtt_cb) {
   LOG_IF(WARNING, config.aes_key.empty() || config.aes_iv_mask.empty())
       << "Unsafe to send video with encryption DISABLED.";
   if (!video_encryptor_.Initialize(config.aes_key, config.aes_iv_mask)) {
@@ -113,11 +130,19 @@ void CastTransportSenderImpl::InitializeVideo(
     video_sender_.reset();
     status_callback_.Run(TRANSPORT_VIDEO_UNINITIALIZED);

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

Need a return statement after this statement.

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

Done.

   }
-}
-
-void CastTransportSenderImpl::SetPacketReceiver(
-    const PacketReceiverCallback& packet_receiver) {
-  transport_->StartReceiving(packet_receiver);
+  video_rtcp_session_.reset(
+      new Rtcp(cast_message_cb,
+               rtt_cb,
+               base::Bind(&CastTransportSenderImpl::OnReceivedLogMessage,
+                          weak_factory_.GetWeakPtr(), VIDEO_EVENT),
+               clock_,
+               &pacer_,
+               config.ssrc,
+               config.feedback_ssrc,
+               config.c_name));
+  // Only start receiving once.
+  if (!audio_sender_)
+    StartReceiving();
 }
 
 namespace {
@@ -152,29 +177,21 @@ void CastTransportSenderImpl::InsertCodedVideoFrame(
 }
 
 void CastTransportSenderImpl::SendRtcpFromRtpSender(
-    uint32 packet_type_flags,
-    uint32 ntp_seconds,
-    uint32 ntp_fraction,
-    uint32 rtp_timestamp,
-    const RtcpDlrrReportBlock& dlrr,
-    uint32 sending_ssrc,
-    const std::string& c_name) {
-  RtcpSenderInfo sender_info;
-  sender_info.ntp_seconds = ntp_seconds;
-  sender_info.ntp_fraction = ntp_fraction;
-  sender_info.rtp_timestamp = rtp_timestamp;
-  if (audio_sender_ && audio_sender_->ssrc() == sending_ssrc) {
-    sender_info.send_packet_count = audio_sender_->send_packet_count();
-    sender_info.send_octet_count = audio_sender_->send_octet_count();
-  } else if (video_sender_ && video_sender_->ssrc() == sending_ssrc) {
-    sender_info.send_packet_count = video_sender_->send_packet_count();
-    sender_info.send_octet_count = video_sender_->send_octet_count();
+    bool audio,
+    base::TimeTicks current_time,
+    uint32 current_time_as_rtp_timestamp) {
+  if (audio_sender_ && audio) {
+    audio_rtcp_session_->SendRtcpFromRtpSender(
+        current_time, current_time_as_rtp_timestamp,
+        audio_sender_->send_packet_count(), audio_sender_->send_octet_count());
+  } else if (video_sender_ && !audio) {
+    video_rtcp_session_->SendRtcpFromRtpSender(
+        current_time, current_time_as_rtp_timestamp,
+        video_sender_->send_packet_count(), video_sender_->send_octet_count());
   } else {
-    LOG(ERROR) << "Sending RTCP with an invalid SSRC.";
+    LOG(ERROR) << "Invalid request for sending RTCP packet.";

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

IMHO, this should should be a NOTREACHED().

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

Done.

     return;

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

You can remove this return statement now.

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

Done.

   }
-  rtcp_builder_.SendRtcpFromRtpSender(
-      packet_type_flags, sender_info, dlrr, sending_ssrc, c_name);
 }
 
 void CastTransportSenderImpl::ResendPackets(
@@ -195,12 +212,79 @@ void CastTransportSenderImpl::ResendPackets(
   }
 }
 
+PacketReceiverCallback CastTransportSenderImpl::PacketReceiverForTesting() {
+  return base::Bind(&CastTransportSenderImpl::OnReceivedPacket,
+                    weak_factory_.GetWeakPtr());
+}
+
 void CastTransportSenderImpl::SendRawEvents() {
   DCHECK(event_subscriber_.get());
   DCHECK(!raw_events_callback_.is_null());
   std::vector<PacketEvent> packet_events;
+  std::vector<FrameEvent> frame_events;
   event_subscriber_->GetPacketEventsAndReset(&packet_events);

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

Hopefully, event_subscriber_ only resets packet event state, and not also the
frame event state.

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

Yes, that's what the contract says.

-  raw_events_callback_.Run(packet_events);
+  event_subscriber_->GetFrameEventsAndReset(&frame_events);
+  raw_events_callback_.Run(packet_events, frame_events);
+}
+
+void CastTransportSenderImpl::StartReceiving() {
+  if (!transport_)
+    return;
+  transport_->StartReceiving(
+      base::Bind(&CastTransportSenderImpl::OnReceivedPacket,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void CastTransportSenderImpl::OnReceivedPacket(scoped_ptr<Packet> packet) {
+  if (audio_rtcp_session_ &&
+      audio_rtcp_session_->IncomingRtcpPacket(&packet->front(),
+                                              packet->size())) {
+    return;
+  }
+  if (video_rtcp_session_ &&
+      video_rtcp_session_->IncomingRtcpPacket(&packet->front(),
+                                              packet->size())) {
+    return;
+  }
+  VLOG(1) << "Stale packet received.";
+}
+
+void CastTransportSenderImpl::OnReceivedLogMessage(
+    EventMediaType media_type,
+    const RtcpReceiverLogMessage& log) {
+  // Add received log messages into our log system.
+  RtcpReceiverLogMessage::const_iterator it = log.begin();
+  for (; it != log.end(); ++it) {
+    uint32 rtp_timestamp = it->rtp_timestamp_;
+
+    RtcpReceiverEventLogMessages::const_iterator event_it =
+        it->event_log_messages_.begin();
+    for (; event_it != it->event_log_messages_.end(); ++event_it) {
+      switch (event_it->type) {
+        case PACKET_RECEIVED:
+          logging_.InsertPacketEvent(
+              event_it->event_timestamp, event_it->type,
+              media_type, rtp_timestamp,
+              kFrameIdUnknown, event_it->packet_id, 0, 0);
+          break;
+        case FRAME_ACK_SENT:
+        case FRAME_DECODED:
+          logging_.InsertFrameEvent(
+              event_it->event_timestamp, event_it->type, media_type,
+              rtp_timestamp, kFrameIdUnknown);
+          break;
+        case FRAME_PLAYOUT:
+          logging_.InsertFrameEventWithDelay(
+              event_it->event_timestamp, event_it->type, media_type,
+              rtp_timestamp, kFrameIdUnknown, event_it->delay_delta);
+          break;
+        default:
+          VLOG(2) << "Received log message via RTCP that we did not expect: "
+                  << static_cast<int>(event_it->type);
+          break;
+      }
+    }
+  }
 }
 
 }  // namespace cast