Remove TimeToSendPacket and TimeToSendPadding from the default module.

Thie CL moves the default RTP module logic for TimeToSendPacket and TimeToSendPadding to PayloadRouter class and asserts on usage of the default module. BUG=769 TEST=New unittest. R=stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/33319004 Cr-Commit-Position: refs/heads/master@{#8383} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8383 4adac7df-926f-26a2-2b94-8c16560cd09d
2015-02-17 10:15:06 +00:00 · 2015-02-17 10:15:06 +00:00 · 290cb56dca
commit 290cb56dca
parent c0fc4dd87c
7 changed files with 216 additions and 52 deletions
--- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@ -501,42 +501,18 @@ bool ModuleRtpRtcpImpl::TimeToSendPacket(uint32_t ssrc,
                                         uint16_t sequence_number,
                                         int64_t capture_time_ms,
                                         bool retransmission) {
-  if (!IsDefaultModule()) {
-    // Don't send from default module.
-    if (SendingMedia() && ssrc == rtp_sender_.SSRC()) {
-      return rtp_sender_.TimeToSendPacket(sequence_number, capture_time_ms,
-                                          retransmission);
-    }
-  } else {
-    CriticalSectionScoped lock(critical_section_module_ptrs_.get());
-    std::vector<ModuleRtpRtcpImpl*>::iterator it = child_modules_.begin();
-    while (it != child_modules_.end()) {
-      if ((*it)->SendingMedia() && ssrc == (*it)->rtp_sender_.SSRC()) {
-        return (*it)->rtp_sender_.TimeToSendPacket(sequence_number,
-                                                   capture_time_ms,
-                                                   retransmission);
-      }
-      ++it;
-    }
+  assert(!IsDefaultModule());
+  if (SendingMedia() && ssrc == rtp_sender_.SSRC()) {
+    return rtp_sender_.TimeToSendPacket(
+        sequence_number, capture_time_ms, retransmission);
  }
  // No RTP sender is interested in sending this packet.
  return true;
 }

 size_t ModuleRtpRtcpImpl::TimeToSendPadding(size_t bytes) {
-  if (!IsDefaultModule()) {
-    // Don't send from default module.
-    return rtp_sender_.TimeToSendPadding(bytes);
-  } else {
-    CriticalSectionScoped lock(critical_section_module_ptrs_.get());
-    for (size_t i = 0; i < child_modules_.size(); ++i) {
-      // Send padding on one of the modules sending media.
-      if (child_modules_[i]->SendingMedia()) {
-        return child_modules_[i]->rtp_sender_.TimeToSendPadding(bytes);
-      }
-    }
-  }
-  return 0;
+  assert(!IsDefaultModule());
+  return rtp_sender_.TimeToSendPadding(bytes);
 }

 bool ModuleRtpRtcpImpl::GetSendSideDelay(int* avg_send_delay_ms,
--- a/webrtc/video_engine/payload_router.cc
+++ b/webrtc/video_engine/payload_router.cc
@ -71,6 +71,29 @@ bool PayloadRouter::RoutePayload(FrameType frame_type,
      payload_length, fragmentation, rtp_video_hdr) == 0 ? true : false;
 }

+bool PayloadRouter::TimeToSendPacket(uint32_t ssrc,
+                                     uint16_t sequence_number,
+                                     int64_t capture_timestamp,
+                                     bool retransmission) {
+  CriticalSectionScoped cs(crit_.get());
+  for (auto* rtp_module : rtp_modules_) {
+    if (rtp_module->SendingMedia() && ssrc == rtp_module->SSRC()) {
+      return rtp_module->TimeToSendPacket(ssrc, sequence_number,
+                                          capture_timestamp, retransmission);
+    }
+  }
+  return true;
+}
+
+size_t PayloadRouter::TimeToSendPadding(size_t bytes) {
+  CriticalSectionScoped cs(crit_.get());
+  for(auto* rtp_module : rtp_modules_) {
+    if (rtp_module->SendingMedia())
+      return rtp_module->TimeToSendPadding(bytes);
+  }
+  return 0;
+}
+
 size_t PayloadRouter::MaxPayloadLength() const {
  size_t min_payload_length = DefaultMaxPayloadLength();
  CriticalSectionScoped cs(crit_.get());
--- a/webrtc/video_engine/payload_router.h
+++ b/webrtc/video_engine/payload_router.h
@ -54,11 +54,23 @@ class PayloadRouter {
                    const RTPFragmentationHeader* fragmentation,
                    const RTPVideoHeader* rtp_video_hdr);

+  // Called when it's time to send a stored packet.
+  bool TimeToSendPacket(uint32_t ssrc,
+                        uint16_t sequence_number,
+                        int64_t capture_timestamp,
+                        bool retransmission);
+
+  // Called when it's time to send padding, returns the number of bytes actually
+  // sent.
+  size_t TimeToSendPadding(size_t bytes);
+
  // Returns the maximum allowed data payload length, given the configured MTU
  // and RTP headers.
  size_t MaxPayloadLength() const;

 private:
+  // TODO(mflodman): When the new video API has launched, remove crit_ and
+  // assume rtp_modules_ will never change during a call.
  scoped_ptr<CriticalSectionWrapper> crit_;

  // Active sending RTP modules, in layer order.
--- a/webrtc/video_engine/payload_router_unittest.cc
+++ b/webrtc/video_engine/payload_router_unittest.cc
@ -161,4 +161,145 @@ TEST_F(PayloadRouterTest, MaxPayloadLength) {
  EXPECT_EQ(kTestMinPayloadLength, payload_router_->MaxPayloadLength());
 }

+TEST_F(PayloadRouterTest, TimeToSendPacket) {
+  MockRtpRtcp rtp_1;
+  MockRtpRtcp rtp_2;
+  std::list<RtpRtcp*> modules;
+  modules.push_back(&rtp_1);
+  modules.push_back(&rtp_2);
+  payload_router_->SetSendingRtpModules(modules);
+
+  const uint16_t kSsrc1 = 1234;
+  uint16_t sequence_number = 17;
+  uint64_t timestamp = 7890;
+  bool retransmission = false;
+
+  // Send on the first module by letting rtp_1 be sending with correct ssrc.
+  EXPECT_CALL(rtp_1, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(rtp_1, SSRC())
+      .Times(1)
+      .WillOnce(Return(kSsrc1));
+  EXPECT_CALL(rtp_1, TimeToSendPacket(kSsrc1, sequence_number, timestamp,
+                                      retransmission))
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(rtp_2, TimeToSendPacket(_, _, _, _))
+      .Times(0);
+  EXPECT_TRUE(payload_router_->TimeToSendPacket(
+      kSsrc1, sequence_number, timestamp, retransmission));
+
+
+  // Send on the second module by letting rtp_2 be sending, but not rtp_1.
+  ++sequence_number;
+  timestamp += 30;
+  retransmission = true;
+  const uint16_t kSsrc2 = 4567;
+  EXPECT_CALL(rtp_1, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(false));
+  EXPECT_CALL(rtp_2, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(rtp_2, SSRC())
+      .Times(1)
+      .WillOnce(Return(kSsrc2));
+  EXPECT_CALL(rtp_1, TimeToSendPacket(_, _, _, _))
+      .Times(0);
+  EXPECT_CALL(rtp_2, TimeToSendPacket(kSsrc2, sequence_number, timestamp,
+                                      retransmission))
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_TRUE(payload_router_->TimeToSendPacket(
+      kSsrc2, sequence_number, timestamp, retransmission));
+
+  // No module is sending, hence no packet should be sent.
+  EXPECT_CALL(rtp_1, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(false));
+  EXPECT_CALL(rtp_1, TimeToSendPacket(_, _, _,_))
+      .Times(0);
+  EXPECT_CALL(rtp_2, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(false));
+  EXPECT_CALL(rtp_2, TimeToSendPacket(_, _, _, _))
+      .Times(0);
+  EXPECT_TRUE(payload_router_->TimeToSendPacket(
+      kSsrc1, sequence_number, timestamp, retransmission));
+
+  // Add a packet with incorrect ssrc and test it's dropped in the router.
+  EXPECT_CALL(rtp_1, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(rtp_1, SSRC())
+      .Times(1)
+      .WillOnce(Return(kSsrc1));
+  EXPECT_CALL(rtp_2, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(rtp_2, SSRC())
+      .Times(1)
+      .WillOnce(Return(kSsrc2));
+  EXPECT_CALL(rtp_1, TimeToSendPacket(_, _, _,_))
+      .Times(0);
+  EXPECT_CALL(rtp_2, TimeToSendPacket(_, _, _, _))
+      .Times(0);
+  EXPECT_TRUE(payload_router_->TimeToSendPacket(
+      kSsrc1 + kSsrc2, sequence_number, timestamp, retransmission));
+}
+
+TEST_F(PayloadRouterTest, TimeToSendPadding) {
+  MockRtpRtcp rtp_1;
+  MockRtpRtcp rtp_2;
+  std::list<RtpRtcp*> modules;
+  modules.push_back(&rtp_1);
+  modules.push_back(&rtp_2);
+  payload_router_->SetSendingRtpModules(modules);
+
+
+  // Default configuration, sending padding on the first sending module.
+  const size_t requested_padding_bytes = 1000;
+  const size_t sent_padding_bytes = 890;
+  EXPECT_CALL(rtp_1, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(rtp_1, TimeToSendPadding(requested_padding_bytes))
+      .Times(1)
+      .WillOnce(Return(sent_padding_bytes));
+  EXPECT_CALL(rtp_2, TimeToSendPadding(_))
+      .Times(0);
+  EXPECT_EQ(sent_padding_bytes,
+            payload_router_->TimeToSendPadding(requested_padding_bytes));
+
+  // Let only the second module be sending and verify the padding request is
+  // routed there.
+  EXPECT_CALL(rtp_1, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(false));
+  EXPECT_CALL(rtp_1, TimeToSendPadding(requested_padding_bytes))
+      .Times(0);
+  EXPECT_CALL(rtp_2, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(rtp_2, TimeToSendPadding(_))
+      .Times(1)
+      .WillOnce(Return(sent_padding_bytes));
+  EXPECT_EQ(sent_padding_bytes,
+            payload_router_->TimeToSendPadding(requested_padding_bytes));
+
+  // No sending module at all.
+   EXPECT_CALL(rtp_1, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(false));
+  EXPECT_CALL(rtp_1, TimeToSendPadding(requested_padding_bytes))
+      .Times(0);
+  EXPECT_CALL(rtp_2, SendingMedia())
+      .Times(1)
+      .WillOnce(Return(false));
+  EXPECT_CALL(rtp_2, TimeToSendPadding(_))
+      .Times(0);
+  EXPECT_EQ(static_cast<size_t>(0),
+            payload_router_->TimeToSendPadding(requested_padding_bytes));
+}
 }  // namespace webrtc
--- a/webrtc/video_engine/vie_channel_manager.cc
+++ b/webrtc/video_engine/vie_channel_manager.cc
@ -117,7 +117,7 @@ int ViEChannelManager::CreateChannel(int* channel_id,
    return -1;
  }
  // Connect the encoder with the send packet router, to enable sending.
-  vie_encoder->SetSendPayloadRouter(
+  vie_encoder->StartThreadsAndSetSendPayloadRouter(
      channel_map_[new_channel_id]->send_payload_router());

  // Add ViEEncoder to EncoderFeedBackObserver.
@ -183,7 +183,7 @@ int ViEChannelManager::CreateChannel(int* channel_id,
      vie_encoder = NULL;
    }
    // Connect the encoder with the send packet router, to enable sending.
-    vie_encoder->SetSendPayloadRouter(
+    vie_encoder->StartThreadsAndSetSendPayloadRouter(
        channel_map_[new_channel_id]->send_payload_router());

    // Register the ViEEncoder to get key frame requests for this channel.
@ -249,9 +249,11 @@ int ViEChannelManager::DeleteChannel(int channel_id) {
        vie_channel->GetStatsObserver());
    group->SetChannelRembStatus(channel_id, false, false, vie_channel);

-    // Remove the feedback if we're owning the encoder.
+    // If we're owning the encoder, remove the feedback and stop all encoding
+    // threads and processing. This must be done before deleting the channel.
    if (vie_encoder->channel_id() == channel_id) {
      group->GetEncoderStateFeedback()->RemoveEncoder(vie_encoder);
+      vie_encoder->StopThreadsAndRemovePayloadRouter();
    }

    unsigned int remote_ssrc = 0;
--- a/webrtc/video_engine/vie_encoder.cc
+++ b/webrtc/video_engine/vie_encoder.cc
@ -107,6 +107,7 @@ class ViEBitrateObserver : public BitrateObserver {
  ViEEncoder* owner_;
 };

+// TODO(mflodman): Move this observer to PayloadRouter class.
 class ViEPacedSenderCallback : public PacedSender::Callback {
 public:
  explicit ViEPacedSenderCallback(ViEEncoder* owner)
@ -191,14 +192,6 @@ bool ViEEncoder::Init() {
  // Enable/disable content analysis: off by default for now.
  vpm_.EnableContentAnalysis(false);

-  if (module_process_thread_.RegisterModule(&vcm_) != 0 ||
-      module_process_thread_.RegisterModule(default_rtp_rtcp_.get()) != 0) {
-    return false;
-  }
-  if (pacer_thread_->RegisterModule(paced_sender_.get()) != 0 ||
-      pacer_thread_->Start() != 0) {
-    return false;
-  }
  if (qm_callback_) {
    delete qm_callback_;
  }
@ -235,9 +228,24 @@ bool ViEEncoder::Init() {
  return true;
 }

-void ViEEncoder::SetSendPayloadRouter(PayloadRouter* send_payload_router) {
+void ViEEncoder::StartThreadsAndSetSendPayloadRouter(
+    PayloadRouter* send_payload_router) {
  DCHECK(send_payload_router_ == NULL);
  send_payload_router_ = send_payload_router;
+
+  module_process_thread_.RegisterModule(&vcm_);
+  module_process_thread_.RegisterModule(default_rtp_rtcp_.get());
+  pacer_thread_->RegisterModule(paced_sender_.get());
+  pacer_thread_->Start();
+}
+
+void ViEEncoder::StopThreadsAndRemovePayloadRouter() {
+  pacer_thread_->Stop();
+  pacer_thread_->DeRegisterModule(paced_sender_.get());
+  module_process_thread_.DeRegisterModule(&vcm_);
+  module_process_thread_.DeRegisterModule(&vpm_);
+  module_process_thread_.DeRegisterModule(default_rtp_rtcp_.get());
+  send_payload_router_ = nullptr;
 }

 ViEEncoder::~ViEEncoder() {
@ -245,11 +253,6 @@ ViEEncoder::~ViEEncoder() {
  if (bitrate_controller_) {
    bitrate_controller_->RemoveBitrateObserver(bitrate_observer_.get());
  }
-  pacer_thread_->Stop();
-  pacer_thread_->DeRegisterModule(paced_sender_.get());
-  module_process_thread_.DeRegisterModule(&vcm_);
-  module_process_thread_.DeRegisterModule(&vpm_);
-  module_process_thread_.DeRegisterModule(default_rtp_rtcp_.get());
  VideoCodingModule::Destroy(&vcm_);
  VideoProcessingModule::Destroy(&vpm_);
  delete qm_callback_;
@ -453,8 +456,8 @@ bool ViEEncoder::TimeToSendPacket(uint32_t ssrc,
                                  uint16_t sequence_number,
                                  int64_t capture_time_ms,
                                  bool retransmission) {
-  return default_rtp_rtcp_->TimeToSendPacket(ssrc, sequence_number,
-                                             capture_time_ms, retransmission);
+  return send_payload_router_->TimeToSendPacket(
+      ssrc, sequence_number, capture_time_ms, retransmission);
 }

 size_t ViEEncoder::TimeToSendPadding(size_t bytes) {
@ -465,7 +468,7 @@ size_t ViEEncoder::TimeToSendPadding(size_t bytes) {
        send_padding_ || video_suspended_ || min_transmit_bitrate_kbps_ > 0;
  }
  if (send_padding) {
-    return default_rtp_rtcp_->TimeToSendPadding(bytes);
+    return send_payload_router_->TimeToSendPadding(bytes);
  }
  return 0;
 }
--- a/webrtc/video_engine/vie_encoder.h
+++ b/webrtc/video_engine/vie_encoder.h
@ -65,8 +65,15 @@ class ViEEncoder

  bool Init();

-  // This function is assumed to be called before any frames are delivered.
-  void SetSendPayloadRouter(PayloadRouter* send_payload_router);
+  // This function is assumed to be called before any frames are delivered and
+  // only once.
+  // Ideally this would be done in Init, but the dependencies between ViEEncoder
+  // and ViEChannel makes it really hard to do in a good way.
+  void StartThreadsAndSetSendPayloadRouter(PayloadRouter* send_payload_router);
+
+  // This function must be called before the corresponding ViEChannel is
+  // deleted.
+  void StopThreadsAndRemovePayloadRouter();

  void SetNetworkTransmissionState(bool is_transmitting);