From 97b6c757bc6f181e136cf35d90b23fc1872afaa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 25 Jul 2019 09:16:34 +0200 Subject: [PATCH] New pacer: make FlexFEC and Video priority equal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Perf dashboard show a very minor change with the new pacer, for tests that use flexfec. I have found that previously fec was in fact prioritized at the same level as video, see eg PacketTypeToPriority() in RTPSender. With the new pacer we put fec in between video and padding. Not sure if this is in fact an actual problem. In the non-loss case the frame latency should actually be slighly lower, but on the other hand if we have loss fec won't be applied until after the full frame has been sent and so we may end up sending NACK before we apply the FEC and recover a packet. Just to avoid any problems let's revert to the old behavior. Bug: webrtc:10633 Change-Id: I9a4210a64165a6e376c0c70ccaa07b0688cc58a5 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/146714 Reviewed-by: Sebastian Jansson Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#28678} --- modules/pacing/paced_sender.cc | 7 ++++--- modules/pacing/paced_sender_unittest.cc | 12 ++++++++---- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index 63e31569f6..18334e2a4a 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -57,12 +57,13 @@ int GetPriorityForType(RtpPacketToSend::Type type) { // Video has "normal" priority, in the old speak. return 2; case RtpPacketToSend::Type::kForwardErrorCorrection: - // Redundancy is OK to drop, but the content is hopefully not useless. - return 3; + // Send redundancy concurrently to video. If it is delayed it might have a + // lower chance of being useful. + return 2; case RtpPacketToSend::Type::kPadding: // Packets that are in themselves likely useless, only sent to keep the // BWE high. - return 4; + return 3; } } diff --git a/modules/pacing/paced_sender_unittest.cc b/modules/pacing/paced_sender_unittest.cc index 30ff00ad32..01023969fb 100644 --- a/modules/pacing/paced_sender_unittest.cc +++ b/modules/pacing/paced_sender_unittest.cc @@ -1479,7 +1479,7 @@ TEST_P(PacedSenderTest, OwnedPacketPrioritizedOnType) { // Insert a packet of each type, from low to high priority. Since priority // is weighted higher than insert order, these should come out of the pacer - // in backwards order. + // in backwards order with the exception of FEC and Video. for (RtpPacketToSend::Type type : {RtpPacketToSend::Type::kPadding, RtpPacketToSend::Type::kForwardErrorCorrection, @@ -1495,12 +1495,16 @@ TEST_P(PacedSenderTest, OwnedPacketPrioritizedOnType) { EXPECT_CALL( callback, SendPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kVideoRtxSsrc)), _)); - EXPECT_CALL( - callback, - SendPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kVideoSsrc)), _)); + + // FEC and video actually have the same priority, so will come out in + // insertion order. EXPECT_CALL( callback, SendPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kFlexFecSsrc)), _)); + EXPECT_CALL( + callback, + SendPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kVideoSsrc)), _)); + EXPECT_CALL( callback, SendPacket(Pointee(Property(&RtpPacketToSend::Ssrc, kVideoRtxSsrc)), _));