RTC-14823: Video thumbnails are frozen when someone is screensharing (#…

…357)

bridge/engine/EngineMixerVideo.cpp

@@ -784,12 +784,23 @@ void EngineMixer::checkVideoBandwidth(const uint64_t timestamp)
     // Local presenter. Sending TMBBR to restrict slides.
     if (presenterStream)
     {
-        const uint32_t slidesLimit = minUplinkEstimate * _config.slides.allocFactor;
+        const uint32_t thumbnailsBitRate = _engineStreamDirector->getBitrateForAllThumbnails();
+        uint32_t slidesLimit = minUplinkEstimate;
+        if (minUplinkEstimate > _config.slides.borrowBandwidthThreshold.get())
+        {
+            // Reserve space for thumbnails and give a margin so not disable the thumbnails if
+            // a participants starts to estimate slight lower bandwidth.
+            // Never goes bellow the borrowBandwidthThreshold so we don't degrade slides too much
+            slidesLimit = std::max(_config.slides.borrowBandwidthThreshold.get(),
+                static_cast<uint32_t>(0.85 * (minUplinkEstimate - thumbnailsBitRate)));
+        }
 
-        logger::info("limiting bitrate for ssrc %u, at %u",
+        logger::info("limiting slides bitrate for ssrc %u at %u, minUplinkEstimate %u, thumbnailsBitRate %u",
             _loggableId.c_str(),
             presenterSimulcastLevel->ssrc,
-            slidesLimit);
+            slidesLimit,
+            minUplinkEstimate,
+            thumbnailsBitRate);
 
         presenterStream->transport.getJobQueue().addJob<SetMaxMediaBitrateJob>(presenterStream->transport,
             presenterStream->localSsrc,

bridge/engine/EngineStreamDirector.cpp

@@ -1,13 +1,55 @@
 #include "EngineStreamDirector.h"
-
-namespace bridge
+namespace
+{
+constexpr uint32_t calculateMaxBitrate(uint32_t base, uint32_t overhead)
 {
+    // Assuming a _lastN of 9 participants. Perhaps this should be dynamic as EngineStreamDirector knows _lastN
+    return base + 8 * overhead;
+}
+
+} // namespace
+
+using namespace bridge;
+
+// Configuration based on
+// https://chromium.googlesource.com/external/webrtc/+/refs/heads/master/video/config/simulcast.cc
+// Calculation based on following assumptions:
+// - Video at 25fps
+// - RTP header 12 bytes + 8 byte extension header (abs-time extension)
+// - Until 9 bytes for VP8 payload descriptor + VP8 payload header
+// - LOW_QUALITY resolution: 320x180. Target bitrate 150kbits/s, 25 packets/s (~750 bytes per packet + headers)
+// - MID_QUALITY resolution: 640x360. Target bitrate 500kbits/s, 50 packets/s (~1250 bytes per packet + headers)
+// - HIGH_QUALITY resolution: 1280x720. Target bitrate 2500kbits/s, 225 packets/s (~ 1389 bytes per packet + headers)
+// - Round up as the layer selection does not take into account sending audio nor RTCP packets
+constexpr const uint32_t EngineStreamDirector::LOW_QUALITY_BITRATE = 160;
+constexpr const uint32_t EngineStreamDirector::MID_QUALITY_BITRATE = 520;
+constexpr const uint32_t EngineStreamDirector::HIGH_QUALITY_BITRATE = 2600;
+
 constexpr EngineStreamDirector::ConfigRow EngineStreamDirector::configLadder[6] = {
-    // BaseRate = 0, PinnedQuality, UnpinnedQuality, OverheadBitrate, MinBitrateMargin, MaxBitrateMargin
-    {2500 - 500, highQuality, midQuality, 500, 2500, 6500},
-    {500 - 500, midQuality, midQuality, 500, 500, 4500},
-    {500 - 100, midQuality, lowQuality, 100, 500, 1300},
-    {100 - 100, lowQuality, lowQuality, 100, 100, 900},
-    {100, lowQuality, dropQuality, 0, 100, 100},
+    // BaseRate = 0, pinnedQuality, unpinnedQuality, overheadBitrate, minBitrateMargin, maxBitrateMargin
+    {HIGH_QUALITY_BITRATE - MID_QUALITY_BITRATE,
+        highQuality,
+        midQuality,
+        MID_QUALITY_BITRATE,
+        HIGH_QUALITY_BITRATE,
+        calculateMaxBitrate(HIGH_QUALITY_BITRATE, MID_QUALITY_BITRATE)}, // 6760
+    {MID_QUALITY_BITRATE - MID_QUALITY_BITRATE,
+        midQuality,
+        midQuality,
+        MID_QUALITY_BITRATE,
+        MID_QUALITY_BITRATE,
+        calculateMaxBitrate(MID_QUALITY_BITRATE, MID_QUALITY_BITRATE)}, // 4680
+    {MID_QUALITY_BITRATE - LOW_QUALITY_BITRATE,
+        midQuality,
+        lowQuality,
+        LOW_QUALITY_BITRATE,
+        MID_QUALITY_BITRATE,
+        calculateMaxBitrate(MID_QUALITY_BITRATE, LOW_QUALITY_BITRATE)}, // 1800
+    {LOW_QUALITY_BITRATE - LOW_QUALITY_BITRATE,
+        lowQuality,
+        lowQuality,
+        LOW_QUALITY_BITRATE,
+        LOW_QUALITY_BITRATE,
+        calculateMaxBitrate(LOW_QUALITY_BITRATE, LOW_QUALITY_BITRATE)}, // 1440
+    {LOW_QUALITY_BITRATE, lowQuality, dropQuality, 0, LOW_QUALITY_BITRATE, LOW_QUALITY_BITRATE},
     {0, dropQuality, dropQuality, 0, 0, 0}};
-} // namespace bridge

bridge/engine/EngineStreamDirector.h

@@ -23,6 +23,10 @@ namespace bridge
 class EngineStreamDirector
 {
 public:
+    static const uint32_t LOW_QUALITY_BITRATE;
+    static const uint32_t MID_QUALITY_BITRATE;
+    static const uint32_t HIGH_QUALITY_BITRATE;
+
     enum QualityLevel : uint32_t
     {
         lowQuality = 0,
@@ -686,16 +690,23 @@ class EngineStreamDirector
 
     bool needsSlidesBitrateAllocation() const { return _slidesSsrc != 0 && _slidesBitrateKbps == 0; }
 
+    uint32_t getBitrateForAllThumbnails() const
+    {
+        const uint32_t slidesCount = _slidesSsrc == 0 ? 0 : 1;
+        return (std::max(slidesCount, std::min(_lastN, static_cast<uint32_t>(_lowQualitySsrcs.size()))) - slidesCount) *
+            LOW_QUALITY_BITRATE;
+    }
+
 private:
     /** All bandwidth values are in kbps. */
     struct ConfigRow
     {
-        const size_t BaseRate;
-        const QualityLevel PinnedQuality;
-        const QualityLevel UnpinnedQuality;
-        const size_t OverheadBitrate;
-        const size_t MinBitrateMargin;
-        const size_t MaxBitrateMargin;
+        const size_t baseRate;
+        const QualityLevel pinnedQuality;
+        const QualityLevel unpinnedQuality;
+        const size_t overheadBitrate;
+        const size_t minBitrateMargin;
+        const size_t maxBitrateMargin;
     };
 
     static const ConfigRow configLadder[6];
@@ -973,10 +984,15 @@ class EngineStreamDirector
         // "maxReceivingVideoStreams" can be 0, if we are the only one sending video, or the very first one in that case
         // quality limits will be initially overestimated (but would be periodically updated with each uplink estimation
         // anyway). To lookup "configLadder" we need at least one stream, thus capping to 1 from below.
+        // We will also not include the _slidesBitrateKbps on the cost calculation for the participant that is sending
+        // the slides
 
-        bool sendingVideo = participantStreams.primary.isSendingVideo() ||
+        const bool sendingVideo = participantStreams.primary.isSendingVideo() ||
             (participantStreams.secondary.isSet() && participantStreams.secondary.get().isSendingVideo());
 
+        const bool sendingSlides = participantStreams.primary.isSendingSlides() ||
+            (participantStreams.secondary.isSet() && participantStreams.secondary.get().isSendingSlides());
+
         const auto maxReceivingVideoStreams = std::max(1ul,
             std::min(std::max(1ul, _lowQualitySsrcs.size()), (unsigned long)_lastN) - (sendingVideo ? 1 : 0));
 
@@ -988,13 +1004,15 @@ class EngineStreamDirector
             (participantStreams.estimatedUplinkBandwidth != 0 ? participantStreams.estimatedUplinkBandwidth
                                                               : _maxDefaultLevelBandwidthKbps);
 
+        const uint32_t allocationBitrateKbpsForSlides = (sendingSlides ? 0 : _slidesBitrateKbps);
+
         for (const auto& config : configLadder)
         {
             const auto configCost =
-                config.BaseRate + maxReceivingVideoStreams * config.OverheadBitrate + _slidesBitrateKbps;
+                config.baseRate + maxReceivingVideoStreams * config.overheadBitrate + allocationBitrateKbpsForSlides;
 
-            assert(configCost >= config.MinBitrateMargin + _slidesBitrateKbps);
-            assert(configCost <= config.MaxBitrateMargin + _slidesBitrateKbps);
+            assert(configCost >= config.minBitrateMargin + allocationBitrateKbpsForSlides);
+            assert(configCost <= config.maxBitrateMargin + allocationBitrateKbpsForSlides);
 
             const auto configIsBetter = bestConfigCost == 0 || configCost > bestConfigCost;
             if (configIsBetter && configCost <= estimatedUplinkBandwidth)
@@ -1005,8 +1023,8 @@ class EngineStreamDirector
             configId++;
         }
         assert(configId == 6);
-        outPinnedQuality = configLadder[bestConfigId].PinnedQuality;
-        outUnpinnedQuality = configLadder[bestConfigId].UnpinnedQuality;
+        outPinnedQuality = configLadder[bestConfigId].pinnedQuality;
+        outUnpinnedQuality = configLadder[bestConfigId].unpinnedQuality;
 
         DIRECTOR_LOG("VQ pinned: %c, unpinned %c, max streams %ld, estimated uplink %d, reserve for slides: %d "
                      "(endpoint %zu)",

config/Config.h

@@ -36,8 +36,8 @@ class Config : public ConfigReader
 
     CFG_GROUP()
     // Value between 0 and 127, where 127 is the lowest audio level and 0 the highest.
-    // Default is 126 to make possible simiulate silence with 127.
-    // If procecessing of silent packet is still desired - set it to 127.
+    // Default is 126 to make possible simulate silence with 127.
+    // If processing of silent packet is still desired - set it to 127.
     CFG_PROP(uint8_t, silenceThresholdLevel, 126);
     CFG_PROP(uint32_t, lastN, 3);
     CFG_PROP(uint32_t, lastNextra, 2);
@@ -95,7 +95,10 @@ class Config : public ConfigReader
 
     CFG_GROUP()
     CFG_PROP(uint32_t, minBitrate, 900);
-    CFG_PROP(double, allocFactor, 1.0);
+    // CFG_PROP(uint32_t, maxBitrate, 7500);
+    CFG_PROP(uint32_t,
+        borrowBandwidthThreshold,
+        2500); // value from which we can borrow slides bandwidth for sending video thumbnails
     CFG_GROUP_END(slides)
 
     CFG_GROUP()

test/bridge/EngineStreamDirectorTest.cpp

@@ -428,14 +428,14 @@ TEST_F(EngineStreamDirectorTest, setUplinkEstimateKbps)
     _engineStreamDirector->updateBandwidthFloor(5, 2, 2);
     _engineStreamDirector->updateBandwidthFloor(5, 3, 2);
 
-    EXPECT_TRUE(_engineStreamDirector->setUplinkEstimateKbps(1, 100, 1 * utils::Time::sec));
+    EXPECT_TRUE(_engineStreamDirector->setUplinkEstimateKbps(1, 160, 1 * utils::Time::sec));
     EXPECT_FALSE(_engineStreamDirector->setUplinkEstimateKbps(1, 1000, 2 * utils::Time::sec));
     EXPECT_TRUE(_engineStreamDirector->setUplinkEstimateKbps(1, 1000, 6 * utils::Time::sec));
-    EXPECT_TRUE(_engineStreamDirector->setUplinkEstimateKbps(1, 100, 7 * utils::Time::sec));
+    EXPECT_TRUE(_engineStreamDirector->setUplinkEstimateKbps(1, 160, 7 * utils::Time::sec));
     EXPECT_FALSE(_engineStreamDirector->setUplinkEstimateKbps(1, 10000, 8 * utils::Time::sec));
     EXPECT_FALSE(_engineStreamDirector->setUplinkEstimateKbps(1, 10000, 9 * utils::Time::sec));
     EXPECT_FALSE(_engineStreamDirector->setUplinkEstimateKbps(1, 10000, 10 * utils::Time::sec));
-    EXPECT_FALSE(_engineStreamDirector->setUplinkEstimateKbps(1, 100, 11 * utils::Time::sec));
+    EXPECT_FALSE(_engineStreamDirector->setUplinkEstimateKbps(1, 160, 11 * utils::Time::sec));
     EXPECT_FALSE(_engineStreamDirector->setUplinkEstimateKbps(1, 10000, 14 * utils::Time::sec));
     EXPECT_TRUE(_engineStreamDirector->setUplinkEstimateKbps(1, 10000, 16 * utils::Time::sec));
 }
@@ -462,11 +462,11 @@ TEST_F(EngineStreamDirectorTest, pinnedMidQualityStreamIsIncludedMidBandwidth)
 TEST_F(EngineStreamDirectorTest, pinnedLowQualityStreamIsIncludedLowBandwidth)
 {
     _engineStreamDirector->addParticipant(1, makeSimulcastStream(1, 2, 3, 4, 5, 6));
-    _engineStreamDirector->setUplinkEstimateKbps(1, 100, 5 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(1, 160, 5 * utils::Time::sec);
     _engineStreamDirector->addParticipant(2, makeSimulcastStream(7, 8, 9, 10, 11, 12));
-    _engineStreamDirector->setUplinkEstimateKbps(2, 100, 5 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(2, 160, 5 * utils::Time::sec);
     _engineStreamDirector->addParticipant(3, makeSimulcastStream(13, 14, 15, 16, 17, 18));
-    _engineStreamDirector->setUplinkEstimateKbps(3, 100, 5 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(3, 160, 5 * utils::Time::sec);
 
     _engineStreamDirector->streamActiveStateChanged(3, 13, true);
     _engineStreamDirector->streamActiveStateChanged(3, 15, true);
@@ -574,8 +574,8 @@ TEST_F(EngineStreamDirectorTest, lowEstimateUsesLowQualityLevel)
     _engineStreamDirector->addParticipant(2);
     _engineStreamDirector->pin(2, 1);
 
-    _engineStreamDirector->setUplinkEstimateKbps(2, 101, 60 * utils::Time::sec);
-    _engineStreamDirector->setUplinkEstimateKbps(2, 101, 61 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(2, 161, 60 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(2, 161, 61 * utils::Time::sec);
 
     EXPECT_TRUE(_engineStreamDirector->isSsrcUsed(1, 1, true, true, 0));
 }
@@ -615,8 +615,8 @@ TEST_F(EngineStreamDirectorTest, bandwidthEstimationAllNeededQualityLevelsAreUse
     _engineStreamDirector->setUplinkEstimateKbps(3, 2000, 61 * utils::Time::sec);
 
     // Low estimate
-    _engineStreamDirector->setUplinkEstimateKbps(4, 101, 60 * utils::Time::sec);
-    _engineStreamDirector->setUplinkEstimateKbps(4, 101, 61 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(4, 161, 60 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(4, 161, 61 * utils::Time::sec);
 
     // Used by 4
     EXPECT_TRUE(_engineStreamDirector->isSsrcUsed(1, 1, true, true, 0));
@@ -666,8 +666,8 @@ TEST_F(EngineStreamDirectorTest, lowEstimateForwardsLowQualityLevel)
     _engineStreamDirector->addParticipant(2);
     _engineStreamDirector->pin(2, 1);
 
-    _engineStreamDirector->setUplinkEstimateKbps(2, 100, 60 * utils::Time::sec);
-    _engineStreamDirector->setUplinkEstimateKbps(2, 100, 61 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(2, 160, 60 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(2, 160, 61 * utils::Time::sec);
 
     EXPECT_TRUE(_engineStreamDirector->shouldForwardSsrc(2, 1));
     EXPECT_FALSE(_engineStreamDirector->shouldForwardSsrc(2, 3));
@@ -713,12 +713,12 @@ TEST_F(EngineStreamDirectorTest, bandwidthEstimationAllNeededQualityLevelsAreFor
     _engineStreamDirector->setUplinkEstimateKbps(3, 2000, 61 * utils::Time::sec);
 
     // Low estimate
-    _engineStreamDirector->setUplinkEstimateKbps(4, 100, 60 * utils::Time::sec);
-    _engineStreamDirector->setUplinkEstimateKbps(4, 100, 61 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(4, 160, 60 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(4, 160, 61 * utils::Time::sec);
 
     // Very low estimate
-    _engineStreamDirector->setUplinkEstimateKbps(5, 99, 60 * utils::Time::sec);
-    _engineStreamDirector->setUplinkEstimateKbps(5, 99, 61 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(5, 159, 60 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(5, 159, 61 * utils::Time::sec);
 
     // Used by 5
     EXPECT_FALSE(_engineStreamDirector->shouldForwardSsrc(5, 1));
@@ -835,7 +835,7 @@ TEST_F(EngineStreamDirectorTest,
     _engineStreamDirector->pin(3, 2);
     _engineStreamDirector->pin(4, 2);
 
-    _engineStreamDirector->setUplinkEstimateKbps(1, 400, 10 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(1, 640, 10 * utils::Time::sec);
 
     EXPECT_TRUE(_engineStreamDirector->isSsrcUsed(13, 3, true, true, 0));
     EXPECT_TRUE(_engineStreamDirector->isSsrcUsed(15, 3, true, true, 0));
@@ -853,10 +853,10 @@ TEST_F(EngineStreamDirectorTest, midQualitySsrc_InLastN_AndNotPinned_AndAllParti
     _engineStreamDirector->pin(3, 2);
     _engineStreamDirector->pin(4, 2);
 
-    _engineStreamDirector->setUplinkEstimateKbps(1, 400, 10 * utils::Time::sec);
-    _engineStreamDirector->setUplinkEstimateKbps(2, 400, 10 * utils::Time::sec);
-    _engineStreamDirector->setUplinkEstimateKbps(3, 400, 10 * utils::Time::sec);
-    _engineStreamDirector->setUplinkEstimateKbps(4, 400, 10 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(1, 640, 10 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(2, 640, 10 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(3, 640, 10 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(4, 640, 10 * utils::Time::sec);
 
     EXPECT_TRUE(_engineStreamDirector->isSsrcUsed(13, 3, true, true, 0));
     EXPECT_FALSE(_engineStreamDirector->isSsrcUsed(15, 3, true, true, 0));
@@ -1211,7 +1211,7 @@ TEST_F(EngineStreamDirectorTest, highQualitySsrcAndMidQualitySsrcs_PinTarget_Hig
     addActiveVideoSender(8, 43);
     addActiveVideoSender(9, 49);
 
-    _engineStreamDirector->setUplinkEstimateKbps(1, 6000, 10 * utils::Time::sec);
+    _engineStreamDirector->setUplinkEstimateKbps(1, 6240, 10 * utils::Time::sec);
     _engineStreamDirector->pin(1, 2);
 
     EXPECT_TRUE(_engineStreamDirector->shouldForwardSsrc(1, 11));