From 1e51a388bcf7b644bf4f1a2fb09363f866c67fcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Wed, 11 Dec 2019 16:47:09 +0100 Subject: [PATCH] Makes padding prefer video SSRCs instead of audio. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#30066} --- modules/pacing/packet_router.cc | 20 ++++++-- modules/pacing/packet_router.h | 7 ++- modules/pacing/packet_router_unittest.cc | 61 +++++++++++++++++++++++- modules/rtp_rtcp/include/rtp_rtcp.h | 3 ++ modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 1 + modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 ++ modules/rtp_rtcp/source/rtp_rtcp_impl.h | 2 + modules/rtp_rtcp/source/rtp_sender.cc | 4 ++ modules/rtp_rtcp/source/rtp_sender.h | 1 + 9 files changed, 94 insertions(+), 10 deletions(-) diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index c280299950..32df52583a 100644 --- a/modules/pacing/packet_router.cc +++ b/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); - send_modules_map_[ssrc] = std::pair::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 @@ void PacketRouter::SendPacket(std::unique_ptr 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> 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); diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 117d681f6c..40b3ad1407 100644 --- a/modules/pacing/packet_router.h +++ b/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_|; - std::unordered_map::iterator>> - send_modules_map_ RTC_GUARDED_BY(modules_crit_); + // Ssrc to RtpRtcp module; + std::unordered_map send_modules_map_ + RTC_GUARDED_BY(modules_crit_); std::list 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 0c95e7fa76..03e9ae9331 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/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>( + kExpectedPaddingPackets); + }; + + NiceMock 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 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 2fea235d34..b3cd8f6418 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/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; diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 6102e0a938..83bc7ccec7 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/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, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 4f851ba8b2..987ae0ec59 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/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( diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index d50b9259e6..976653a458 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/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, diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 6010d033b7..c993e47c2e 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/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; diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index cb59bb2e5f..8915e39e9e 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/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);