Makes padding prefer video SSRCs instead of audio.

Some clients will not count audio packets into the bandwidth estimate
despite negotiating e.g. abs-send-time for that SSRC.
If padding is sent on such an RTP module, we might get stuck in a low
resolution.

This CL works around that by preferring to send padding on video SSRCs.

Bug: webrtc:11196
Change-Id: I1ff503a31a85bc32315006a4f15f8b08e5d4e883
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/161941
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30066}

modules/pacing/packet_router.cc

@@ -74,15 +74,22 @@ void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate) {
 
 void PacketRouter::AddSendRtpModuleToMap(RtpRtcp* rtp_module, uint32_t ssrc) {
   RTC_DCHECK(send_modules_map_.find(ssrc) == send_modules_map_.end());
-  send_modules_list_.push_front(rtp_module);
+  // Always keep the audio modules at the back of the list, so that when we
-  send_modules_map_[ssrc] = std::pair<RtpRtcp*, std::list<RtpRtcp*>::iterator>(
+  // iterate over the modules in order to find one that can send padding we
-      rtp_module, send_modules_list_.begin());
+  // will prioritize video. This is important to make sure they are counted
+  // into the bandwidth estimate properly.
+  if (rtp_module->IsAudioConfigured()) {
+    send_modules_list_.push_back(rtp_module);
+  } else {
+    send_modules_list_.push_front(rtp_module);
+  }
+  send_modules_map_[ssrc] = rtp_module;
 }
 
 void PacketRouter::RemoveSendRtpModuleFromMap(uint32_t ssrc) {
   auto kv = send_modules_map_.find(ssrc);
   RTC_DCHECK(kv != send_modules_map_.end());
-  send_modules_list_.erase(kv->second.second);
+  send_modules_list_.remove(kv->second);
   send_modules_map_.erase(kv);
 }
 
@@ -146,7 +153,7 @@ void PacketRouter::SendPacket(std::unique_ptr<RtpPacketToSend> packet,
     return;
   }
 
-  RtpRtcp* rtp_module = kv->second.first;
+  RtpRtcp* rtp_module = kv->second;
   if (!rtp_module->TrySendPacket(packet.get(), cluster_info)) {
     RTC_LOG(LS_WARNING) << "Failed to send packet, rejected by RTP module.";
     return;
@@ -177,6 +184,9 @@ std::vector<std::unique_ptr<RtpPacketToSend>> PacketRouter::GeneratePadding(
     }
   }
 
+  // Iterate over all modules send module. Video modules will be at the front
+  // and so will be prioritized. This is important since audio packets may not
+  // be taken into account by the bandwidth estimator, e.g. in FF.
   for (RtpRtcp* rtp_module : send_modules_list_) {
     if (rtp_module->SupportsPadding()) {
       padding_packets = rtp_module->GeneratePadding(target_size_bytes);

modules/pacing/packet_router.h

@@ -94,10 +94,9 @@ class PacketRouter : public RemoteBitrateObserver,
       RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_);
 
   rtc::CriticalSection modules_crit_;
-  // Ssrc to RtpRtcp module and iterator into |send_modules_list_|;
+  // Ssrc to RtpRtcp module;
-  std::unordered_map<uint32_t,
+  std::unordered_map<uint32_t, RtpRtcp*> send_modules_map_
-                     std::pair<RtpRtcp*, std::list<RtpRtcp*>::iterator>>
+      RTC_GUARDED_BY(modules_crit_);
-      send_modules_map_ RTC_GUARDED_BY(modules_crit_);
   std::list<RtpRtcp*> send_modules_list_ RTC_GUARDED_BY(modules_crit_);
   // The last module used to send media.
   RtpRtcp* last_send_module_ RTC_GUARDED_BY(modules_crit_);

modules/pacing/packet_router_unittest.cc

@@ -95,7 +95,7 @@ TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_SendTransportFeedback) {
   EXPECT_FALSE(packet_router_.SendCombinedRtcpPacket(std::move(feedback)));
 }
 
-TEST_F(PacketRouterTest, GeneratePaddingPicksCorrectModule) {
+TEST_F(PacketRouterTest, GeneratePaddingPrioritizesRtx) {
   // Two RTP modules. The first (prioritized due to rtx) isn't sending media so
   // should not be called.
   const uint16_t kSsrc1 = 1234;
@@ -129,6 +129,65 @@ TEST_F(PacketRouterTest, GeneratePaddingPicksCorrectModule) {
   packet_router_.RemoveSendRtpModule(&rtp_2);
 }
 
+TEST_F(PacketRouterTest, GeneratePaddingPrioritizesVideo) {
+  // Two RTP modules. Neither support RTX, both support padding,
+  // but the first one is for audio and second for video.
+  const uint16_t kSsrc1 = 1234;
+  const uint16_t kSsrc2 = 4567;
+  const size_t kPaddingSize = 123;
+  const size_t kExpectedPaddingPackets = 1;
+
+  auto generate_padding = [&](size_t padding_size) {
+    return std::vector<std::unique_ptr<RtpPacketToSend>>(
+        kExpectedPaddingPackets);
+  };
+
+  NiceMock<MockRtpRtcp> audio_module;
+  ON_CALL(audio_module, RtxSendStatus()).WillByDefault(Return(kRtxOff));
+  ON_CALL(audio_module, SSRC()).WillByDefault(Return(kSsrc1));
+  ON_CALL(audio_module, SupportsPadding).WillByDefault(Return(true));
+  ON_CALL(audio_module, IsAudioConfigured).WillByDefault(Return(true));
+
+  NiceMock<MockRtpRtcp> video_module;
+  ON_CALL(video_module, RtxSendStatus()).WillByDefault(Return(kRtxOff));
+  ON_CALL(video_module, SSRC()).WillByDefault(Return(kSsrc2));
+  ON_CALL(video_module, SupportsPadding).WillByDefault(Return(true));
+  ON_CALL(video_module, IsAudioConfigured).WillByDefault(Return(false));
+
+  // First add only the audio module. Since this is the only choice we have,
+  // padding should be sent on the audio ssrc.
+  packet_router_.AddSendRtpModule(&audio_module, false);
+  EXPECT_CALL(audio_module, GeneratePadding(kPaddingSize))
+      .WillOnce(generate_padding);
+  packet_router_.GeneratePadding(kPaddingSize);
+
+  // Add the video module, this should now be prioritized since we cannot
+  // guarantee that audio packets will be included in the BWE.
+  packet_router_.AddSendRtpModule(&video_module, false);
+  EXPECT_CALL(audio_module, GeneratePadding).Times(0);
+  EXPECT_CALL(video_module, GeneratePadding(kPaddingSize))
+      .WillOnce(generate_padding);
+  packet_router_.GeneratePadding(kPaddingSize);
+
+  // Remove and the add audio module again. Module order shouldn't matter;
+  // video should still be prioritized.
+  packet_router_.RemoveSendRtpModule(&audio_module);
+  packet_router_.AddSendRtpModule(&audio_module, false);
+  EXPECT_CALL(audio_module, GeneratePadding).Times(0);
+  EXPECT_CALL(video_module, GeneratePadding(kPaddingSize))
+      .WillOnce(generate_padding);
+  packet_router_.GeneratePadding(kPaddingSize);
+
+  // Remove and the video module, we should fall back to padding on the
+  // audio module again.
+  packet_router_.RemoveSendRtpModule(&video_module);
+  EXPECT_CALL(audio_module, GeneratePadding(kPaddingSize))
+      .WillOnce(generate_padding);
+  packet_router_.GeneratePadding(kPaddingSize);
+
+  packet_router_.RemoveSendRtpModule(&audio_module);
+}
+
 TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) {
   const uint16_t kSsrc1 = 1234;
   const uint16_t kSsrc2 = 4567;

modules/rtp_rtcp/include/rtp_rtcp.h

@@ -250,6 +250,9 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface {
   // Returns current media sending status.
   virtual bool SendingMedia() const = 0;
 
+  // Returns whether audio is configured (i.e. Configuration::audio = true).
+  virtual bool IsAudioConfigured() const = 0;
+
   // Indicate that the packets sent by this module should be counted towards the
   // bitrate estimate since the stream participates in the bitrate allocation.
   virtual void SetAsPartOfAllocation(bool part_of_allocation) = 0;

modules/rtp_rtcp/mocks/mock_rtp_rtcp.h

@@ -77,6 +77,7 @@ class MockRtpRtcp : public RtpRtcp {
   MOCK_CONST_METHOD0(Sending, bool());
   MOCK_METHOD1(SetSendingMediaStatus, void(bool sending));
   MOCK_CONST_METHOD0(SendingMedia, bool());
+  MOCK_CONST_METHOD0(IsAudioConfigured, bool());
   MOCK_METHOD1(SetAsPartOfAllocation, void(bool));
   MOCK_CONST_METHOD4(BitrateSent,
                      void(uint32_t* total_rate,

modules/rtp_rtcp/source/rtp_rtcp_impl.cc

@@ -334,6 +334,11 @@ bool ModuleRtpRtcpImpl::SendingMedia() const {
   return rtp_sender_ ? rtp_sender_->packet_generator.SendingMedia() : false;
 }
 
+bool ModuleRtpRtcpImpl::IsAudioConfigured() const {
+  return rtp_sender_ ? rtp_sender_->packet_generator.IsAudioConfigured()
+                     : false;
+}
+
 void ModuleRtpRtcpImpl::SetAsPartOfAllocation(bool part_of_allocation) {
   RTC_CHECK(rtp_sender_);
   rtp_sender_->packet_sender.ForceIncludeSendPacketsInAllocation(

modules/rtp_rtcp/source/rtp_rtcp_impl.h

@@ -125,6 +125,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp {
 
   bool SendingMedia() const override;
 
+  bool IsAudioConfigured() const override;
+
   void SetAsPartOfAllocation(bool part_of_allocation) override;
 
   bool OnSendingRtpFrame(uint32_t timestamp,

modules/rtp_rtcp/source/rtp_sender.cc

@@ -545,6 +545,10 @@ bool RTPSender::SendingMedia() const {
   return sending_media_;
 }
 
+bool RTPSender::IsAudioConfigured() const {
+  return audio_configured_;
+}
+
 void RTPSender::SetTimestampOffset(uint32_t timestamp) {
   rtc::CritScope lock(&send_critsect_);
   timestamp_offset_ = timestamp;

modules/rtp_rtcp/source/rtp_sender.h

@@ -54,6 +54,7 @@ class RTPSender {
 
   void SetSendingMediaStatus(bool enabled);
   bool SendingMedia() const;
+  bool IsAudioConfigured() const;
 
   uint32_t TimestampOffset() const;
   void SetTimestampOffset(uint32_t timestamp);