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}
diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc
index c280299..32df525 100644
--- a/modules/pacing/packet_router.cc
+++ b/modules/pacing/packet_router.cc
@@ -74,15 +74,22 @@
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);
- send_modules_map_[ssrc] = std::pair<RtpRtcp*, std::list<RtpRtcp*>::iterator>(
- rtp_module, send_modules_list_.begin());
+ // Always keep the audio modules at the back of the list, so that when we
+ // iterate over the modules in order to find one that can send padding we
+ // 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 @@
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 @@
}
}
+ // 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);
diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h
index 117d681..40b3ad1 100644
--- a/modules/pacing/packet_router.h
+++ b/modules/pacing/packet_router.h
@@ -94,10 +94,9 @@
RTC_EXCLUSIVE_LOCKS_REQUIRED(modules_crit_);
rtc::CriticalSection modules_crit_;
- // Ssrc to RtpRtcp module and iterator into |send_modules_list_|;
- std::unordered_map<uint32_t,
- std::pair<RtpRtcp*, std::list<RtpRtcp*>::iterator>>
- send_modules_map_ RTC_GUARDED_BY(modules_crit_);
+ // Ssrc to RtpRtcp module;
+ std::unordered_map<uint32_t, RtpRtcp*> 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_);
diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc
index 0c95e7f..03e9ae9 100644
--- a/modules/pacing/packet_router_unittest.cc
+++ b/modules/pacing/packet_router_unittest.cc
@@ -95,7 +95,7 @@
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 @@
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;
diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h
index 2fea235..b3cd8f6 100644
--- a/modules/rtp_rtcp/include/rtp_rtcp.h
+++ b/modules/rtp_rtcp/include/rtp_rtcp.h
@@ -250,6 +250,9 @@
// 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;
diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
index 6102e0a..83bc7cc 100644
--- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
+++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
@@ -77,6 +77,7 @@
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,
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
index 4f851ba..987ae0e 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -334,6 +334,11 @@
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(
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
index d50b925..976653a 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
@@ -125,6 +125,8 @@
bool SendingMedia() const override;
+ bool IsAudioConfigured() const override;
+
void SetAsPartOfAllocation(bool part_of_allocation) override;
bool OnSendingRtpFrame(uint32_t timestamp,
diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc
index 6010d03..c993e47 100644
--- a/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/modules/rtp_rtcp/source/rtp_sender.cc
@@ -545,6 +545,10 @@
return sending_media_;
}
+bool RTPSender::IsAudioConfigured() const {
+ return audio_configured_;
+}
+
void RTPSender::SetTimestampOffset(uint32_t timestamp) {
rtc::CritScope lock(&send_critsect_);
timestamp_offset_ = timestamp;
diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h
index cb59bb2..8915e39 100644
--- a/modules/rtp_rtcp/source/rtp_sender.h
+++ b/modules/rtp_rtcp/source/rtp_sender.h
@@ -54,6 +54,7 @@
void SetSendingMediaStatus(bool enabled);
bool SendingMedia() const;
+ bool IsAudioConfigured() const;
uint32_t TimestampOffset() const;
void SetTimestampOffset(uint32_t timestamp);