From 576db1bf60dd11691e022b7b722ecbd9bb929b7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Mon, 8 Jun 2020 13:32:20 +0200 Subject: [PATCH] Fixes incorrect padding setting for VP9 SVC. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unit test added to verify root cause is fixed. Scenario test added to verify high-level behavior. Bug: webrtc:11654 Change-Id: I1ad6e2750f5272e86b4198749edbbf5dfd8315c4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176564 Commit-Queue: Erik Språng Reviewed-by: Erik Språng Reviewed-by: Sebastian Jansson Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#31462} --- test/scenario/video_stream_unittest.cc | 74 +++++++++ video/video_send_stream_impl.cc | 29 ++-- video/video_send_stream_impl_unittest.cc | 199 ++++++++++++----------- 3 files changed, 195 insertions(+), 107 deletions(-) diff --git a/test/scenario/video_stream_unittest.cc b/test/scenario/video_stream_unittest.cc index 1f2cad7e8c..37afe1b1e7 100644 --- a/test/scenario/video_stream_unittest.cc +++ b/test/scenario/video_stream_unittest.cc @@ -169,5 +169,79 @@ TEST(VideoStreamTest, SendsFecWithFlexFec) { VideoSendStream::Stats video_stats = video->send()->GetStats(); EXPECT_GT(video_stats.substreams.begin()->second.rtp_stats.fec.packets, 0u); } + +TEST(VideoStreamTest, ResolutionAdaptsToAvailableBandwidth) { + // Declared before scenario to avoid use after free. + std::atomic num_qvga_frames_(0); + std::atomic num_vga_frames_(0); + + Scenario s; + // Link has enough capacity for VGA. + NetworkSimulationConfig net_conf; + net_conf.bandwidth = DataRate::KilobitsPerSec(800); + net_conf.delay = TimeDelta::Millis(50); + auto* client = s.CreateClient("send", [&](CallClientConfig* c) { + c->transport.rates.start_rate = DataRate::KilobitsPerSec(800); + }); + auto send_net = {s.CreateSimulationNode(net_conf)}; + auto ret_net = {s.CreateSimulationNode(net_conf)}; + auto* route = s.CreateRoutes( + client, send_net, s.CreateClient("return", CallClientConfig()), ret_net); + + s.CreateVideoStream(route->forward(), [&](VideoStreamConfig* c) { + c->hooks.frame_pair_handlers = {[&](const VideoFramePair& info) { + if (info.decoded->width() == 640) { + ++num_vga_frames_; + } else if (info.decoded->width() == 320) { + ++num_qvga_frames_; + } else { + ADD_FAILURE() << "Unexpected resolution: " << info.decoded->width(); + } + }}; + c->source.framerate = 30; + // The resolution must be high enough to allow smaller layers to be + // created. + c->source.generator.width = 640; + c->source.generator.height = 480; + c->encoder.implementation = CodecImpl::kSoftware; + c->encoder.codec = Codec::kVideoCodecVP9; + // Enable SVC. + c->encoder.layers.spatial = 2; + }); + + // Run for a few seconds, until streams have stabilized, + // check that we are sending VGA. + s.RunFor(TimeDelta::Seconds(5)); + EXPECT_GT(num_vga_frames_, 0u); + + // Trigger cross traffic, run until we have seen 3 consecutive + // seconds with no VGA frames due to reduced available bandwidth. + auto cross_traffic = + s.net()->StartFakeTcpCrossTraffic(send_net, ret_net, FakeTcpConfig()); + + int num_seconds_without_vga = 0; + int num_iterations = 0; + do { + ASSERT_LE(++num_iterations, 100); + num_qvga_frames_ = 0; + num_vga_frames_ = 0; + s.RunFor(TimeDelta::Seconds(1)); + if (num_qvga_frames_ > 0 && num_vga_frames_ == 0) { + ++num_seconds_without_vga; + } else { + num_seconds_without_vga = 0; + } + } while (num_seconds_without_vga < 3); + + // Stop cross traffic, make sure we recover and get VGA frames agian. + s.net()->StopCrossTraffic(cross_traffic); + num_qvga_frames_ = 0; + num_vga_frames_ = 0; + + s.RunFor(TimeDelta::Seconds(40)); + EXPECT_GT(num_qvga_frames_, 0u); + EXPECT_GT(num_vga_frames_, 0u); +} + } // namespace test } // namespace webrtc diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index 03c9613ab4..712af87a0c 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -92,17 +92,26 @@ int CalculateMaxPadBitrateBps(const std::vector& streams, const double hysteresis_factor = RateControlSettings::ParseFromFieldTrials() .GetSimulcastHysteresisFactor(content_type); - const size_t top_active_stream_idx = active_streams.size() - 1; - pad_up_to_bitrate_bps = std::min( - static_cast( - hysteresis_factor * - active_streams[top_active_stream_idx].min_bitrate_bps + - 0.5), - active_streams[top_active_stream_idx].target_bitrate_bps); + if (is_svc) { + // For SVC, since there is only one "stream", the padding bitrate + // needed to enable the top spatial layer is stored in the + // |target_bitrate_bps| field. + // TODO(sprang): This behavior needs to die. + pad_up_to_bitrate_bps = static_cast( + hysteresis_factor * active_streams[0].target_bitrate_bps + 0.5); + } else { + const size_t top_active_stream_idx = active_streams.size() - 1; + pad_up_to_bitrate_bps = std::min( + static_cast( + hysteresis_factor * + active_streams[top_active_stream_idx].min_bitrate_bps + + 0.5), + active_streams[top_active_stream_idx].target_bitrate_bps); - // Add target_bitrate_bps of the lower active streams. - for (size_t i = 0; i < top_active_stream_idx; ++i) { - pad_up_to_bitrate_bps += active_streams[i].target_bitrate_bps; + // Add target_bitrate_bps of the lower active streams. + for (size_t i = 0; i < top_active_stream_idx; ++i) { + pad_up_to_bitrate_bps += active_streams[i].target_bitrate_bps; + } } } } else if (!active_streams.empty() && pad_to_min_bitrate) { diff --git a/video/video_send_stream_impl_unittest.cc b/video/video_send_stream_impl_unittest.cc index 178d3865b7..bb702ba270 100644 --- a/video/video_send_stream_impl_unittest.cc +++ b/video/video_send_stream_impl_unittest.cc @@ -10,6 +10,7 @@ #include "video/video_send_stream_impl.h" +#include #include #include @@ -43,6 +44,8 @@ bool operator==(const BitrateAllocationUpdate& a, namespace internal { namespace { using ::testing::_; +using ::testing::AllOf; +using ::testing::Field; using ::testing::Invoke; using ::testing::NiceMock; using ::testing::Return; @@ -907,112 +910,114 @@ TEST_F(VideoSendStreamImplTest, KeepAliveOnDroppedFrame) { ASSERT_TRUE(done.Wait(5000)); } -TEST_F(VideoSendStreamImplTest, ConfiguresBitratesForSvcWithAlr) { - test_queue_.SendTask( - [this] { - const bool kSuspend = false; - config_.suspend_below_min_bitrate = kSuspend; - config_.rtp.extensions.emplace_back( - RtpExtension::kTransportSequenceNumberUri, 1); - config_.periodic_alr_bandwidth_probing = true; - auto vss_impl = CreateVideoSendStreamImpl( - kDefaultInitialBitrateBps, kDefaultBitratePriority, - VideoEncoderConfig::ContentType::kScreen); - vss_impl->Start(); +TEST_F(VideoSendStreamImplTest, ConfiguresBitratesForSvc) { + struct TestConfig { + bool screenshare = false; + bool alr = false; + int min_padding_bitrate_bps = 0; + }; - // Svc - VideoStream stream; - stream.width = 1920; - stream.height = 1080; - stream.max_framerate = 30; - stream.min_bitrate_bps = 60000; - stream.target_bitrate_bps = 6000000; - stream.max_bitrate_bps = 1250000; - stream.num_temporal_layers = 2; - stream.max_qp = 56; - stream.bitrate_priority = 1; + std::vector test_variants; + for (bool screenshare : {false, true}) { + for (bool alr : {false, true}) { + for (int min_padding : {0, 400000}) { + test_variants.push_back({screenshare, alr, min_padding}); + } + } + } - int min_transmit_bitrate_bps = 400000; + for (const TestConfig& test_config : test_variants) { + test_queue_.SendTask( + [this, test_config] { + const bool kSuspend = false; + config_.suspend_below_min_bitrate = kSuspend; + config_.rtp.extensions.emplace_back( + RtpExtension::kTransportSequenceNumberUri, 1); + config_.periodic_alr_bandwidth_probing = test_config.alr; + auto vss_impl = CreateVideoSendStreamImpl( + kDefaultInitialBitrateBps, kDefaultBitratePriority, + test_config.screenshare + ? VideoEncoderConfig::ContentType::kScreen + : VideoEncoderConfig::ContentType::kRealtimeVideo); + vss_impl->Start(); - config_.rtp.ssrcs.emplace_back(1); - config_.rtp.ssrcs.emplace_back(2); + // Svc + VideoStream stream; + stream.width = 1920; + stream.height = 1080; + stream.max_framerate = 30; + stream.min_bitrate_bps = 60000; + stream.target_bitrate_bps = 6000000; + stream.max_bitrate_bps = 1250000; + stream.num_temporal_layers = 2; + stream.max_qp = 56; + stream.bitrate_priority = 1; - EXPECT_CALL(bitrate_allocator_, AddObserver(vss_impl.get(), _)) - .WillRepeatedly(Invoke([&](BitrateAllocatorObserver*, - MediaStreamAllocationConfig config) { - EXPECT_EQ(config.min_bitrate_bps, - static_cast(stream.min_bitrate_bps)); - EXPECT_EQ(config.max_bitrate_bps, - static_cast(stream.max_bitrate_bps)); - if (config.pad_up_bitrate_bps != 0) { - EXPECT_EQ(config.pad_up_bitrate_bps, - static_cast(min_transmit_bitrate_bps)); - } - EXPECT_EQ(config.enforce_min_bitrate, !kSuspend); - })); + config_.rtp.ssrcs.emplace_back(1); + config_.rtp.ssrcs.emplace_back(2); - static_cast(vss_impl.get()) - ->OnEncoderConfigurationChanged( - std::vector{stream}, true, - VideoEncoderConfig::ContentType::kScreen, - min_transmit_bitrate_bps); - vss_impl->Stop(); - }, - RTC_FROM_HERE); -} + EXPECT_CALL( + bitrate_allocator_, + AddObserver( + vss_impl.get(), + AllOf(Field(&MediaStreamAllocationConfig::min_bitrate_bps, + static_cast(stream.min_bitrate_bps)), + Field(&MediaStreamAllocationConfig::max_bitrate_bps, + static_cast(stream.max_bitrate_bps)), + // Stream not yet active - no padding. + Field(&MediaStreamAllocationConfig::pad_up_bitrate_bps, + 0u), + Field(&MediaStreamAllocationConfig::enforce_min_bitrate, + !kSuspend)))); -TEST_F(VideoSendStreamImplTest, ConfiguresBitratesForSvcNoAlr) { - test_queue_.SendTask( - [this] { - const bool kSuspend = false; - config_.suspend_below_min_bitrate = kSuspend; - config_.rtp.extensions.emplace_back( - RtpExtension::kTransportSequenceNumberUri, 1); - config_.periodic_alr_bandwidth_probing = false; - auto vss_impl = CreateVideoSendStreamImpl( - kDefaultInitialBitrateBps, kDefaultBitratePriority, - VideoEncoderConfig::ContentType::kScreen); - vss_impl->Start(); + static_cast(vss_impl.get()) + ->OnEncoderConfigurationChanged( + std::vector{stream}, true, + test_config.screenshare + ? VideoEncoderConfig::ContentType::kScreen + : VideoEncoderConfig::ContentType::kRealtimeVideo, + test_config.min_padding_bitrate_bps); + ::testing::Mock::VerifyAndClearExpectations(&bitrate_allocator_); - // Svc - VideoStream stream; - stream.width = 1920; - stream.height = 1080; - stream.max_framerate = 30; - stream.min_bitrate_bps = 60000; - stream.target_bitrate_bps = 6000000; - stream.max_bitrate_bps = 1250000; - stream.num_temporal_layers = 2; - stream.max_qp = 56; - stream.bitrate_priority = 1; + // Simulate an encoded image, this will turn the stream active and + // enable padding. + EncodedImage encoded_image; + CodecSpecificInfo codec_specific; + EXPECT_CALL(rtp_video_sender_, OnEncodedImage) + .WillRepeatedly(Return(EncodedImageCallback::Result( + EncodedImageCallback::Result::OK))); - int min_transmit_bitrate_bps = 400000; + // Screensharing implicitly forces ALR. + const bool using_alr = test_config.alr || test_config.screenshare; + // If ALR is used, pads only to min bitrate as rampup is handled by + // probing. Otherwise target_bitrate contains the padding target. + int expected_padding = + using_alr ? stream.min_bitrate_bps : stream.target_bitrate_bps; + // Min padding bitrate may override padding target. + expected_padding = + std::max(expected_padding, test_config.min_padding_bitrate_bps); + EXPECT_CALL( + bitrate_allocator_, + AddObserver( + vss_impl.get(), + AllOf(Field(&MediaStreamAllocationConfig::min_bitrate_bps, + static_cast(stream.min_bitrate_bps)), + Field(&MediaStreamAllocationConfig::max_bitrate_bps, + static_cast(stream.max_bitrate_bps)), + // Stream now active - min bitrate use as padding target + // when ALR is active. + Field(&MediaStreamAllocationConfig::pad_up_bitrate_bps, + expected_padding), + Field(&MediaStreamAllocationConfig::enforce_min_bitrate, + !kSuspend)))); + static_cast(vss_impl.get()) + ->OnEncodedImage(encoded_image, &codec_specific, nullptr); + ::testing::Mock::VerifyAndClearExpectations(&bitrate_allocator_); - config_.rtp.ssrcs.emplace_back(1); - config_.rtp.ssrcs.emplace_back(2); - - EXPECT_CALL(bitrate_allocator_, AddObserver(vss_impl.get(), _)) - .WillRepeatedly(Invoke([&](BitrateAllocatorObserver*, - MediaStreamAllocationConfig config) { - EXPECT_EQ(config.min_bitrate_bps, - static_cast(stream.min_bitrate_bps)); - EXPECT_EQ(config.max_bitrate_bps, - static_cast(stream.max_bitrate_bps)); - if (config.pad_up_bitrate_bps != 0) { - EXPECT_EQ(config.pad_up_bitrate_bps, - static_cast(stream.target_bitrate_bps)); - } - EXPECT_EQ(config.enforce_min_bitrate, !kSuspend); - })); - - static_cast(vss_impl.get()) - ->OnEncoderConfigurationChanged( - std::vector{stream}, true, - VideoEncoderConfig::ContentType::kScreen, - min_transmit_bitrate_bps); - vss_impl->Stop(); - }, - RTC_FROM_HERE); + vss_impl->Stop(); + }, + RTC_FROM_HERE); + } } } // namespace internal } // namespace webrtc