From 6e3510f7c93cde680449e18e693b4a6ae0415881 Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Fri, 15 May 2020 14:25:59 +0200 Subject: [PATCH 01/12] Merge to M84: Fallback to software decoders on consequtive decode errors on key-frames TBR=brandtr@webrtc.org (cherry picked from commit 35fc1537afa92e0b08589af055421d14f81e2fe1) Bug: webrtc:11575, chromium:1084963 Change-Id: I09be17ab5155e9f610c8f7c451ca52d7d65e24d1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175222 Commit-Queue: Ilya Nikolaevskiy Reviewed-by: Rasmus Brandt Cr-Original-Commit-Position: refs/heads/master@{#31295} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175902 Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/branch-heads/4147@{#1} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- ...oder_software_fallback_wrapper_unittest.cc | 62 +++++++++++++++++++ ...video_decoder_software_fallback_wrapper.cc | 22 +++++-- 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/api/video_codecs/test/video_decoder_software_fallback_wrapper_unittest.cc b/api/video_codecs/test/video_decoder_software_fallback_wrapper_unittest.cc index ee61893563..30d5287c94 100644 --- a/api/video_codecs/test/video_decoder_software_fallback_wrapper_unittest.cc +++ b/api/video_codecs/test/video_decoder_software_fallback_wrapper_unittest.cc @@ -218,6 +218,68 @@ TEST_F(VideoDecoderSoftwareFallbackWrapperTest, fallback_wrapper_->Release(); } +TEST_F(VideoDecoderSoftwareFallbackWrapperTest, FallbacksOnTooManyErrors) { + VideoCodec codec = {}; + fallback_wrapper_->InitDecode(&codec, 2); + + fake_decoder_->decode_return_code_ = WEBRTC_VIDEO_CODEC_ERROR; + EncodedImage encoded_image; + encoded_image._frameType = VideoFrameType::kVideoFrameKey; + // Doesn't fallback from a single error. + fallback_wrapper_->Decode(encoded_image, false, -1); + EXPECT_STREQ("fake-decoder", fallback_wrapper_->ImplementationName()); + + // However, many frames with the same error, fallback should happen. + const int kNumFramesToEncode = 10; + for (int i = 0; i < kNumFramesToEncode; ++i) { + fallback_wrapper_->Decode(encoded_image, false, -1); + } + // Hard coded expected value since libvpx is the software implementation name + // for VP8. Change accordingly if the underlying implementation does. + EXPECT_STREQ("libvpx (fallback from: fake-decoder)", + fallback_wrapper_->ImplementationName()); + fallback_wrapper_->Release(); +} + +TEST_F(VideoDecoderSoftwareFallbackWrapperTest, + DoesNotFallbackOnDeltaFramesErrors) { + VideoCodec codec = {}; + fallback_wrapper_->InitDecode(&codec, 2); + + fake_decoder_->decode_return_code_ = WEBRTC_VIDEO_CODEC_ERROR; + EncodedImage encoded_image; + encoded_image._frameType = VideoFrameType::kVideoFrameDelta; + + // Many decoded frames with the same error + const int kNumFramesToEncode = 10; + for (int i = 0; i < kNumFramesToEncode; ++i) { + fallback_wrapper_->Decode(encoded_image, false, -1); + } + EXPECT_STREQ("fake-decoder", fallback_wrapper_->ImplementationName()); + + fallback_wrapper_->Release(); +} + +TEST_F(VideoDecoderSoftwareFallbackWrapperTest, + DoesNotFallbacksOnNonConsequtiveErrors) { + VideoCodec codec = {}; + fallback_wrapper_->InitDecode(&codec, 2); + + EncodedImage encoded_image; + encoded_image._frameType = VideoFrameType::kVideoFrameKey; + + const int kNumFramesToEncode = 10; + for (int i = 0; i < kNumFramesToEncode; ++i) { + // Interleaved errors and successful decodes. + fake_decoder_->decode_return_code_ = WEBRTC_VIDEO_CODEC_ERROR; + fallback_wrapper_->Decode(encoded_image, false, -1); + fake_decoder_->decode_return_code_ = WEBRTC_VIDEO_CODEC_OK; + fallback_wrapper_->Decode(encoded_image, false, -1); + } + EXPECT_STREQ("fake-decoder", fallback_wrapper_->ImplementationName()); + fallback_wrapper_->Release(); +} + class ForcedSoftwareDecoderFallbackTest : public VideoDecoderSoftwareFallbackWrapperTest { public: diff --git a/api/video_codecs/video_decoder_software_fallback_wrapper.cc b/api/video_codecs/video_decoder_software_fallback_wrapper.cc index f78d9b885f..128087f207 100644 --- a/api/video_codecs/video_decoder_software_fallback_wrapper.cc +++ b/api/video_codecs/video_decoder_software_fallback_wrapper.cc @@ -30,6 +30,8 @@ namespace webrtc { namespace { +constexpr size_t kMaxConsequtiveHwErrors = 4; + class VideoDecoderSoftwareFallbackWrapper final : public VideoDecoder { public: VideoDecoderSoftwareFallbackWrapper( @@ -74,6 +76,7 @@ class VideoDecoderSoftwareFallbackWrapper final : public VideoDecoder { const std::string fallback_implementation_name_; DecodedImageCallback* callback_; int32_t hw_decoded_frames_since_last_fallback_; + size_t hw_consequtive_generic_errors_; }; VideoDecoderSoftwareFallbackWrapper::VideoDecoderSoftwareFallbackWrapper( @@ -86,7 +89,8 @@ VideoDecoderSoftwareFallbackWrapper::VideoDecoderSoftwareFallbackWrapper( std::string(fallback_decoder_->ImplementationName()) + " (fallback from: " + hw_decoder_->ImplementationName() + ")"), callback_(nullptr), - hw_decoded_frames_since_last_fallback_(0) {} + hw_decoded_frames_since_last_fallback_(0), + hw_consequtive_generic_errors_(0) {} VideoDecoderSoftwareFallbackWrapper::~VideoDecoderSoftwareFallbackWrapper() = default; @@ -196,14 +200,24 @@ int32_t VideoDecoderSoftwareFallbackWrapper::Decode( int32_t ret = WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; ret = hw_decoder_->Decode(input_image, missing_frames, render_time_ms); if (ret != WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE) { - if (ret == WEBRTC_VIDEO_CODEC_OK) { + if (ret != WEBRTC_VIDEO_CODEC_ERROR) { ++hw_decoded_frames_since_last_fallback_; + hw_consequtive_generic_errors_ = 0; + return ret; + } + if (input_image._frameType == VideoFrameType::kVideoFrameKey) { + // Only count errors on key-frames, since generic errors can happen + // with hw decoder due to many arbitrary reasons. + // However, requesting a key-frame is supposed to fix the issue. + ++hw_consequtive_generic_errors_; + } + if (hw_consequtive_generic_errors_ < kMaxConsequtiveHwErrors) { + return ret; } - return ret; } // HW decoder returned WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE or - // initialization failed, fallback to software. + // too many generic errors on key-frames encountered. if (!InitFallbackDecoder()) { return ret; } From b8678c5f5304c5a73bfba5af6867ce0f9c0edf0a Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Fri, 15 May 2020 12:24:29 +0200 Subject: [PATCH 02/12] Merge to M84: Log decoder implementation name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TBR=sprang@webrtc.org (cherry picked from commit 43c108b7e9a7436d43e0eb36f5898a432b4d840e) Bug: chromium:1084963 Change-Id: I2c6b6a2a62bbcd058b8ed336e6e0f36b8b0d0844 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175220 Reviewed-by: Erik Språng Commit-Queue: Ilya Nikolaevskiy Cr-Original-Commit-Position: refs/heads/master@{#31321} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175903 Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/branch-heads/4147@{#2} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- modules/video_coding/decoder_database.cc | 6 ++++-- modules/video_coding/generic_decoder.cc | 14 +++++++++++--- modules/video_coding/generic_decoder.h | 2 ++ 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/modules/video_coding/decoder_database.cc b/modules/video_coding/decoder_database.cc index c203721e5d..38a18baa6d 100644 --- a/modules/video_coding/decoder_database.cc +++ b/modules/video_coding/decoder_database.cc @@ -169,8 +169,10 @@ std::unique_ptr VCMDecoderDataBase::CreateAndInitDecoder( decoder_item->settings->width = frame.EncodedImage()._encodedWidth; decoder_item->settings->height = frame.EncodedImage()._encodedHeight; } - if (ptr_decoder->InitDecode(decoder_item->settings.get(), - decoder_item->number_of_cores) < 0) { + int err = ptr_decoder->InitDecode(decoder_item->settings.get(), + decoder_item->number_of_cores); + if (err < 0) { + RTC_LOG(LS_ERROR) << "Failed to initialize decoder. Error code: " << err; return nullptr; } memcpy(new_codec, decoder_item->settings.get(), sizeof(VideoCodec)); diff --git a/modules/video_coding/generic_decoder.cc b/modules/video_coding/generic_decoder.cc index ca9b5e2d47..cfe16ed3f1 100644 --- a/modules/video_coding/generic_decoder.cc +++ b/modules/video_coding/generic_decoder.cc @@ -211,7 +211,10 @@ int32_t VCMGenericDecoder::InitDecode(const VideoCodec* settings, TRACE_EVENT0("webrtc", "VCMGenericDecoder::InitDecode"); _codecType = settings->codecType; - return decoder_->InitDecode(settings, numberOfCores); + int err = decoder_->InitDecode(settings, numberOfCores); + implementation_name_ = decoder_->ImplementationName(); + RTC_LOG(LS_INFO) << "Decoder implementation: " << implementation_name_; + return err; } int32_t VCMGenericDecoder::Decode(const VCMEncodedFrame& frame, Timestamp now) { @@ -239,8 +242,13 @@ int32_t VCMGenericDecoder::Decode(const VCMEncodedFrame& frame, Timestamp now) { _nextFrameInfoIdx = (_nextFrameInfoIdx + 1) % kDecoderFrameMemoryLength; int32_t ret = decoder_->Decode(frame.EncodedImage(), frame.MissingFrame(), frame.RenderTimeMs()); - - _callback->OnDecoderImplementationName(decoder_->ImplementationName()); + const char* new_implementation_name = decoder_->ImplementationName(); + if (new_implementation_name != implementation_name_) { + implementation_name_ = new_implementation_name; + RTC_LOG(LS_INFO) << "Changed decoder implementation to: " + << new_implementation_name; + } + _callback->OnDecoderImplementationName(implementation_name_.c_str()); if (ret < WEBRTC_VIDEO_CODEC_OK) { RTC_LOG(LS_WARNING) << "Failed to decode frame with timestamp " << frame.Timestamp() << ", error code: " << ret; diff --git a/modules/video_coding/generic_decoder.h b/modules/video_coding/generic_decoder.h index 4b4d83ecd5..40fe667d65 100644 --- a/modules/video_coding/generic_decoder.h +++ b/modules/video_coding/generic_decoder.h @@ -12,6 +12,7 @@ #define MODULES_VIDEO_CODING_GENERIC_DECODER_H_ #include +#include #include "api/units/time_delta.h" #include "modules/video_coding/encoded_frame.h" @@ -112,6 +113,7 @@ class VCMGenericDecoder { VideoCodecType _codecType; const bool _isExternal; VideoContentType _last_keyframe_content_type; + std::string implementation_name_; }; } // namespace webrtc From f9e2d036a3a6173af1205e7823966c13e516cdde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Wed, 3 Jun 2020 10:47:19 +0200 Subject: [PATCH 03/12] Merge to M84: Fixes excessive stats updating in TaskQueuePacedSender. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge note: Production code is a plain merge, unit tests needed some back-porting due to refactoring of test harness in tot. TaskQueuePacedSender::MaybeUpdateStats() is intended to be called when packets are sent or by a sequence of "scheduled" calls. There should only be one scheduled call in flight at a time - and that one reschedules itself if needed when it runs. A bug however caused the "schedules task in flight" flag to incorrectly be set to false, leading to more and more schedules tasks being alive - eating CPU cycles. This CL fixes that and also makes sure the queue time properly goes down to zero before the next idle interval check, even if there are no more packets to send. (cherry picked from commit 998524a08e085f3180740642946a489a74b520e9) Bug: webrtc:10809 Change-Id: I4e13fcf95619a43dcaf0ed38bce9684a5b0d8d5e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176330 Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Erik Språng Cr-Original-Commit-Position: refs/heads/master@{#31390} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176409 Cr-Commit-Position: refs/branch-heads/4147@{#3} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- modules/pacing/pacing_controller.cc | 4 +- modules/pacing/task_queue_paced_sender.cc | 68 +++++-- modules/pacing/task_queue_paced_sender.h | 5 +- .../task_queue_paced_sender_unittest.cc | 180 ++++++++++++++---- 4 files changed, 201 insertions(+), 56 deletions(-) diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc index f21e63733f..4b4fb0bd26 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc @@ -399,7 +399,9 @@ void PacingController::ProcessPackets() { if (target_send_time.IsMinusInfinity()) { target_send_time = now; } else if (now < target_send_time) { - // We are too early, abort and regroup! + // We are too early, but if queue is empty still allow draining some debt. + TimeDelta elapsed_time = UpdateTimeAndGetElapsed(now); + UpdateBudgetWithElapsedTime(elapsed_time); return; } diff --git a/modules/pacing/task_queue_paced_sender.cc b/modules/pacing/task_queue_paced_sender.cc index a4ce9fe9d6..16d6df554c 100644 --- a/modules/pacing/task_queue_paced_sender.cc +++ b/modules/pacing/task_queue_paced_sender.cc @@ -174,6 +174,11 @@ TimeDelta TaskQueuePacedSender::OldestPacketWaitTime() const { return GetStats().oldest_packet_wait_time; } +void TaskQueuePacedSender::OnStatsUpdated(const Stats& stats) { + rtc::CritScope cs(&stats_crit_); + current_stats_ = stats; +} + void TaskQueuePacedSender::MaybeProcessPackets( Timestamp scheduled_process_time) { RTC_DCHECK_RUN_ON(&task_queue_); @@ -230,40 +235,61 @@ void TaskQueuePacedSender::SendRtpPacket( void TaskQueuePacedSender::MaybeUpdateStats(bool is_scheduled_call) { if (is_shutdown_) { + if (is_scheduled_call) { + stats_update_scheduled_ = false; + } return; } Timestamp now = clock_->CurrentTime(); - if (!is_scheduled_call && - now - last_stats_time_ < kMinTimeBetweenStatsUpdates) { - // Too frequent unscheduled stats update, return early. - return; + if (is_scheduled_call) { + // Allow scheduled task to process packets to clear up an remaining debt + // level in an otherwise empty queue. + pacing_controller_.ProcessPackets(); + } else { + if (now - last_stats_time_ < kMinTimeBetweenStatsUpdates) { + // Too frequent unscheduled stats update, return early. + return; + } } - rtc::CritScope cs(&stats_crit_); - current_stats_.expected_queue_time = pacing_controller_.ExpectedQueueTime(); - current_stats_.first_sent_packet_time = - pacing_controller_.FirstSentPacketTime(); - current_stats_.oldest_packet_wait_time = - pacing_controller_.OldestPacketWaitTime(); - current_stats_.queue_size = pacing_controller_.QueueSizeData(); + Stats new_stats; + new_stats.expected_queue_time = pacing_controller_.ExpectedQueueTime(); + new_stats.first_sent_packet_time = pacing_controller_.FirstSentPacketTime(); + new_stats.oldest_packet_wait_time = pacing_controller_.OldestPacketWaitTime(); + new_stats.queue_size = pacing_controller_.QueueSizeData(); + OnStatsUpdated(new_stats); + last_stats_time_ = now; bool pacer_drained = pacing_controller_.QueueSizePackets() == 0 && pacing_controller_.CurrentBufferLevel().IsZero(); // If there's anything interesting to get from the pacer and this is a - // scheduled call (no scheduled call in flight), post a new scheduled stats + // scheduled call (or no scheduled call in flight), post a new scheduled stats // update. - if (!pacer_drained && (is_scheduled_call || !stats_update_scheduled_)) { - task_queue_.PostDelayedTask( - [this]() { - RTC_DCHECK_RUN_ON(&task_queue_); - MaybeUpdateStats(true); - }, - kMaxTimeBetweenStatsUpdates.ms()); - stats_update_scheduled_ = true; - } else { + if (!pacer_drained) { + if (!stats_update_scheduled_) { + // There is no pending delayed task to update stats, add one. + // Treat this call as being scheduled in order to bootstrap scheduling + // loop. + stats_update_scheduled_ = true; + is_scheduled_call = true; + } + + // Only if on the scheduled call loop do we want to schedule a new delayed + // task. + if (is_scheduled_call) { + task_queue_.PostDelayedTask( + [this]() { + RTC_DCHECK_RUN_ON(&task_queue_); + MaybeUpdateStats(true); + }, + kMaxTimeBetweenStatsUpdates.ms()); + } + } else if (is_scheduled_call) { + // This is a scheduled call, signing out since there's nothing interesting + // left to check. stats_update_scheduled_ = false; } } diff --git a/modules/pacing/task_queue_paced_sender.h b/modules/pacing/task_queue_paced_sender.h index 8b47f5ee3d..3f53f00097 100644 --- a/modules/pacing/task_queue_paced_sender.h +++ b/modules/pacing/task_queue_paced_sender.h @@ -99,7 +99,8 @@ class TaskQueuePacedSender : public RtpPacketPacer, // specified by SetPacingRates() if needed to achieve this goal. void SetQueueTimeLimit(TimeDelta limit) override; - private: + protected: + // Exposed as protected for test. struct Stats { Stats() : oldest_packet_wait_time(TimeDelta::Zero()), @@ -110,7 +111,9 @@ class TaskQueuePacedSender : public RtpPacketPacer, TimeDelta expected_queue_time; absl::optional first_sent_packet_time; }; + virtual void OnStatsUpdated(const Stats& stats); + private: // Check if it is time to send packets, or schedule a delayed task if not. // Use Timestamp::MinusInfinity() to indicate that this call has _not_ // been scheduled by the pacing controller. If this is the case, check if diff --git a/modules/pacing/task_queue_paced_sender_unittest.cc b/modules/pacing/task_queue_paced_sender_unittest.cc index ba2aad21ff..83aa73e9aa 100644 --- a/modules/pacing/task_queue_paced_sender_unittest.cc +++ b/modules/pacing/task_queue_paced_sender_unittest.cc @@ -44,6 +44,60 @@ class MockPacketRouter : public PacketRouter { GeneratePadding, std::vector>(size_t target_size_bytes)); }; + +class TaskQueuePacedSenderForTest : public TaskQueuePacedSender { + public: + TaskQueuePacedSenderForTest(Clock* clock, + PacketRouter* packet_router, + RtcEventLog* event_log, + const WebRtcKeyValueConfig* field_trials, + TaskQueueFactory* task_queue_factory) + : TaskQueuePacedSender(clock, + packet_router, + event_log, + field_trials, + task_queue_factory) {} + + void OnStatsUpdated(const Stats& stats) override { + ++num_stats_updates_; + TaskQueuePacedSender::OnStatsUpdated(stats); + } + + size_t num_stats_updates_ = 0; +}; + +std::unique_ptr BuildRtpPacket(RtpPacketMediaType type) { + auto packet = std::make_unique(nullptr); + packet->set_packet_type(type); + switch (type) { + case RtpPacketMediaType::kAudio: + packet->SetSsrc(kAudioSsrc); + break; + case RtpPacketMediaType::kVideo: + packet->SetSsrc(kVideoSsrc); + break; + case RtpPacketMediaType::kRetransmission: + case RtpPacketMediaType::kPadding: + packet->SetSsrc(kVideoRtxSsrc); + break; + case RtpPacketMediaType::kForwardErrorCorrection: + packet->SetSsrc(kFlexFecSsrc); + break; + } + + packet->SetPayloadSize(kDefaultPacketSize); + return packet; +} + +std::vector> GeneratePackets( + RtpPacketMediaType type, + size_t num_packets) { + std::vector> packets; + for (size_t i = 0; i < num_packets; ++i) { + packets.push_back(BuildRtpPacket(type)); + } + return packets; +} } // namespace namespace test { @@ -59,39 +113,6 @@ class TaskQueuePacedSenderTest : public ::testing::Test { time_controller_.GetTaskQueueFactory()) {} protected: - std::unique_ptr BuildRtpPacket(RtpPacketMediaType type) { - auto packet = std::make_unique(nullptr); - packet->set_packet_type(type); - switch (type) { - case RtpPacketMediaType::kAudio: - packet->SetSsrc(kAudioSsrc); - break; - case RtpPacketMediaType::kVideo: - packet->SetSsrc(kVideoSsrc); - break; - case RtpPacketMediaType::kRetransmission: - case RtpPacketMediaType::kPadding: - packet->SetSsrc(kVideoRtxSsrc); - break; - case RtpPacketMediaType::kForwardErrorCorrection: - packet->SetSsrc(kFlexFecSsrc); - break; - } - - packet->SetPayloadSize(kDefaultPacketSize); - return packet; - } - - std::vector> GeneratePackets( - RtpPacketMediaType type, - size_t num_packets) { - std::vector> packets; - for (size_t i = 0; i < num_packets; ++i) { - packets.push_back(BuildRtpPacket(type)); - } - return packets; - } - Timestamp CurrentTime() { return time_controller_.GetClock()->CurrentTime(); } GlobalSimulatedTimeController time_controller_; @@ -196,5 +217,98 @@ TEST_F(TaskQueuePacedSenderTest, SendsAudioImmediately) { ::testing::Mock::VerifyAndClearExpectations(&packet_router_); } +TEST(TaskQueuePacedSenderTestNew, RespectedMinTimeBetweenStatsUpdates) { + GlobalSimulatedTimeController time_controller(Timestamp::Millis(1234)); + MockPacketRouter packet_router; + TaskQueuePacedSenderForTest pacer(time_controller.GetClock(), &packet_router, + /*event_log=*/nullptr, + /*field_trials=*/nullptr, + time_controller.GetTaskQueueFactory()); + const DataRate kPacingDataRate = DataRate::KilobitsPerSec(300); + pacer.SetPacingRates(kPacingDataRate, DataRate::Zero()); + + const TimeDelta kMinTimeBetweenStatsUpdates = TimeDelta::Millis(1); + + // Nothing inserted, no stats updates yet. + EXPECT_EQ(pacer.num_stats_updates_, 0u); + + // Insert one packet, stats should be updated. + pacer.EnqueuePackets(GeneratePackets(RtpPacketMediaType::kVideo, 1)); + time_controller.AdvanceTime(TimeDelta::Zero()); + EXPECT_EQ(pacer.num_stats_updates_, 1u); + + // Advance time half of the min stats update interval, and trigger a + // refresh - stats should not be updated yet. + time_controller.AdvanceTime(kMinTimeBetweenStatsUpdates / 2); + pacer.EnqueuePackets({}); + time_controller.AdvanceTime(TimeDelta::Zero()); + EXPECT_EQ(pacer.num_stats_updates_, 1u); + + // Advance time the next half, now stats update is triggered. + time_controller.AdvanceTime(kMinTimeBetweenStatsUpdates / 2); + pacer.EnqueuePackets({}); + time_controller.AdvanceTime(TimeDelta::Zero()); + EXPECT_EQ(pacer.num_stats_updates_, 2u); +} + +TEST(TaskQueuePacedSenderTestNew, ThrottlesStatsUpdates) { + GlobalSimulatedTimeController time_controller(Timestamp::Millis(1234)); + MockPacketRouter packet_router; + TaskQueuePacedSenderForTest pacer(time_controller.GetClock(), &packet_router, + /*event_log=*/nullptr, + /*field_trials=*/nullptr, + time_controller.GetTaskQueueFactory()); + + // Set rates so one packet adds 10ms of buffer level. + const DataSize kPacketSize = DataSize::Bytes(kDefaultPacketSize); + const TimeDelta kPacketPacingTime = TimeDelta::Millis(10); + const DataRate kPacingDataRate = kPacketSize / kPacketPacingTime; + const TimeDelta kMinTimeBetweenStatsUpdates = TimeDelta::Millis(1); + const TimeDelta kMaxTimeBetweenStatsUpdates = TimeDelta::Millis(33); + + // Nothing inserted, no stats updates yet. + size_t num_expected_stats_updates = 0; + EXPECT_EQ(pacer.num_stats_updates_, num_expected_stats_updates); + pacer.SetPacingRates(kPacingDataRate, DataRate::Zero()); + time_controller.AdvanceTime(kMinTimeBetweenStatsUpdates); + // Updating pacing rates refreshes stats. + EXPECT_EQ(pacer.num_stats_updates_, ++num_expected_stats_updates); + + // Record time when we insert first packet, this triggers the scheduled + // stats updating. + Clock* const clock = time_controller.GetClock(); + const Timestamp start_time = clock->CurrentTime(); + + while (clock->CurrentTime() - start_time <= + kMaxTimeBetweenStatsUpdates - kPacketPacingTime) { + // Enqueue packet, expect stats update. + pacer.EnqueuePackets(GeneratePackets(RtpPacketMediaType::kVideo, 1)); + time_controller.AdvanceTime(TimeDelta::Zero()); + EXPECT_EQ(pacer.num_stats_updates_, ++num_expected_stats_updates); + + // Advance time to halfway through pacing time, expect another stats + // update. + time_controller.AdvanceTime(kPacketPacingTime / 2); + pacer.EnqueuePackets({}); + time_controller.AdvanceTime(TimeDelta::Zero()); + EXPECT_EQ(pacer.num_stats_updates_, ++num_expected_stats_updates); + + // Advance time the rest of the way. + time_controller.AdvanceTime(kPacketPacingTime / 2); + } + + // At this point, the pace queue is drained so there is no more intersting + // update to be made - but there is still as schduled task that should run + // |kMaxTimeBetweenStatsUpdates| after the first update. + time_controller.AdvanceTime(start_time + kMaxTimeBetweenStatsUpdates - + clock->CurrentTime()); + EXPECT_EQ(pacer.num_stats_updates_, ++num_expected_stats_updates); + + // Advance time a significant time - don't expect any more calls as stats + // updating does not happen when queue is drained. + time_controller.AdvanceTime(TimeDelta::Millis(400)); + EXPECT_EQ(pacer.num_stats_updates_, num_expected_stats_updates); +} + } // namespace test } // namespace webrtc From 7b5ff805ebf9b0629946395f40ac38d347caefd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Thu, 4 Jun 2020 09:12:17 +0200 Subject: [PATCH 04/12] [Stats] Don't attempt to aggregate empty VideoSenderInfos. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a crash that could happen if substreams exist but there is no kMedia substream yet. There was an assumption that we either had no substreams or at least one kMedia substream, but this was not true. The correct thing to do is to ignore substream stats that are not associated with any kMedia substream, because we only produce "outbound-rtp" stats objects for existing kMedia substreams. A test is added to make sure no stats are returned. Prior to the fix, this test would crash. (cherry picked from commit e917379c5b15827f26ab0bdd37abebd0c6a56ff2) Bug: chromium:1090712 Change-Id: Ib1f8494a162542ae56bdd2df7618775a3473419b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176446 Commit-Queue: Henrik Boström Reviewed-by: Erik Språng Cr-Original-Commit-Position: refs/heads/master@{#31442} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176751 Reviewed-by: Henrik Boström Cr-Commit-Position: refs/branch-heads/4147@{#4} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- media/engine/webrtc_video_engine.cc | 4 +++- media/engine/webrtc_video_engine_unittest.cc | 21 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 71a0939cb9..3976a6a1c5 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -1587,6 +1587,8 @@ void WebRtcVideoChannel::FillSenderStats(VideoMediaInfo* video_media_info, send_streams_.begin(); it != send_streams_.end(); ++it) { auto infos = it->second->GetPerLayerVideoSenderInfos(log_stats); + if (infos.empty()) + continue; video_media_info->aggregated_senders.push_back( it->second->GetAggregatedVideoSenderInfo(infos)); for (auto&& info : infos) { @@ -2594,7 +2596,7 @@ VideoSenderInfo WebRtcVideoChannel::WebRtcVideoSendStream::GetAggregatedVideoSenderInfo( const std::vector& infos) const { RTC_DCHECK_RUN_ON(&thread_checker_); - RTC_DCHECK(!infos.empty()); + RTC_CHECK(!infos.empty()); if (infos.size() == 1) { return infos[0]; } diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index ce36073449..ae6f15d8af 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -5595,6 +5595,27 @@ TEST_F(WebRtcVideoChannelTest, GetPerLayerStatsReportForSubStreams) { EXPECT_EQ(sender.rid, absl::nullopt); } +TEST_F(WebRtcVideoChannelTest, MediaSubstreamMissingProducesEmpyStats) { + FakeVideoSendStream* stream = AddSendStream(); + + const uint32_t kRtxSsrc = 123u; + const uint32_t kMissingMediaSsrc = 124u; + + // Set up a scenarios where we have a substream that is not kMedia (in this + // case: kRtx) but its associated kMedia stream does not exist yet. This + // results in zero GetPerLayerVideoSenderInfos despite non-empty substreams. + // Covers https://crbug.com/1090712. + auto stats = GetInitialisedStats(); + auto& substream = stats.substreams[kRtxSsrc]; + substream.type = webrtc::VideoSendStream::StreamStats::StreamType::kRtx; + substream.referenced_media_ssrc = kMissingMediaSsrc; + stream->SetStats(stats); + + cricket::VideoMediaInfo video_media_info; + ASSERT_TRUE(channel_->GetStats(&video_media_info)); + EXPECT_TRUE(video_media_info.senders.empty()); +} + TEST_F(WebRtcVideoChannelTest, GetStatsReportsUpperResolution) { FakeVideoSendStream* stream = AddSendStream(); webrtc::VideoSendStream::Stats stats; From 0ddafe3328529a3228ec595fded380a7db2de581 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 05/12] Merge to M84: 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. (cherry picked from commit 576db1bf60dd11691e022b7b722ecbd9bb929b7c) Bug: webrtc:11654, chromium:1092476 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-Original-Commit-Position: refs/heads/master@{#31462} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176840 Cr-Commit-Position: refs/branch-heads/4147@{#5} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- test/scenario/video_stream_unittest.cc | 74 ++++++++ video/video_send_stream_impl.cc | 31 ++-- video/video_send_stream_impl_unittest.cc | 213 ++++++++++++----------- 3 files changed, 203 insertions(+), 115 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); - - // 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; + 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; + } } } } 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 532e035e2b..a0f1201cbd 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; @@ -895,112 +898,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(); - - // 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; - - int min_transmit_bitrate_bps = 400000; - - 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(min_transmit_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); -} - -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(); - - // 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; - - int min_transmit_bitrate_bps = 400000; - - config_.rtp.ssrcs.emplace_back(1); - config_.rtp.ssrcs.emplace_back(2); +TEST_F(VideoSendStreamImplTest, ConfiguresBitratesForSvc) { + struct TestConfig { + bool screenshare = false; + bool alr = false; + int min_padding_bitrate_bps = 0; + }; + + 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}); + } + } + } - 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); - })); + 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(); + + // 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; + + config_.rtp.ssrcs.emplace_back(1); + config_.rtp.ssrcs.emplace_back(2); + + 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)))); + + 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_); + + // 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))); + + // 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_); - 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 From 1b649a889950362a1e59e08264fdd2a9e3e88552 Mon Sep 17 00:00:00 2001 From: Marina Ciocea Date: Fri, 29 May 2020 12:37:01 +0200 Subject: [PATCH 06/12] [M84][InsertableStreams] Send transformed frames on worker queue. When video frame encoding is done on an external thread (for example in the case of hardware encoders), the WebRTC TaskQueueBase::Current() is null; in this case use the worker queue instead to send transformed frames. (cherry picked from commit 2e69660b3e628bb86839cee75c54d9f7cb98cde3) Bug: chromium:1086373 Change-Id: I903ddc52ad6832557fc5b5f76396fe26cf5a88f3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176303 Reviewed-by: Magnus Flodman Reviewed-by: Danil Chapovalov Commit-Queue: Marina Ciocea Cr-Original-Commit-Position: refs/heads/master@{#31388} TBR: marinaciocea@webrtc.org Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176844 Commit-Queue: Guido Urdaneta Reviewed-by: Guido Urdaneta Cr-Commit-Position: refs/branch-heads/4147@{#6} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- call/rtp_video_sender.cc | 2 ++ modules/rtp_rtcp/source/rtp_sender_video.cc | 3 ++- modules/rtp_rtcp/source/rtp_sender_video.h | 2 ++ ...tp_sender_video_frame_transformer_delegate.cc | 16 ++++++++++++---- ...rtp_sender_video_frame_transformer_delegate.h | 4 +++- 5 files changed, 21 insertions(+), 6 deletions(-) diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index b6cb054488..ca8baee2b0 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -30,6 +30,7 @@ #include "rtc_base/checks.h" #include "rtc_base/location.h" #include "rtc_base/logging.h" +#include "rtc_base/task_queue.h" namespace webrtc { @@ -281,6 +282,7 @@ std::vector CreateRtpStreamSenders( video_config.fec_overhead_bytes = fec_generator->MaxPacketOverhead(); } video_config.frame_transformer = frame_transformer; + video_config.worker_queue = transport->GetWorkerQueue()->Get(); auto sender_video = std::make_unique(video_config); rtp_streams.emplace_back(std::move(rtp_rtcp), std::move(sender_video), std::move(fec_generator)); diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index b903b9f001..02c4eb8d28 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -144,7 +144,8 @@ RTPSenderVideo::RTPSenderVideo(const Config& config) RTPSenderVideoFrameTransformerDelegate>( this, config.frame_transformer, - rtp_sender_->SSRC()) + rtp_sender_->SSRC(), + config.worker_queue) : nullptr) { if (frame_transformer_delegate_) frame_transformer_delegate_->Init(); diff --git a/modules/rtp_rtcp/source/rtp_sender_video.h b/modules/rtp_rtcp/source/rtp_sender_video.h index bf5f181823..216f16faf6 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.h +++ b/modules/rtp_rtcp/source/rtp_sender_video.h @@ -20,6 +20,7 @@ #include "api/array_view.h" #include "api/frame_transformer_interface.h" #include "api/scoped_refptr.h" +#include "api/task_queue/task_queue_base.h" #include "api/transport/rtp/dependency_descriptor.h" #include "api/video/video_codec_type.h" #include "api/video/video_frame_type.h" @@ -81,6 +82,7 @@ class RTPSenderVideo { absl::optional red_payload_type; const WebRtcKeyValueConfig* field_trials = nullptr; rtc::scoped_refptr frame_transformer; + TaskQueueBase* worker_queue = nullptr; }; explicit RTPSenderVideo(const Config& config); diff --git a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc index 25ebd1b64c..51fbdb0165 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc @@ -105,10 +105,12 @@ class TransformableVideoSenderFrame : public TransformableVideoFrameInterface { RTPSenderVideoFrameTransformerDelegate::RTPSenderVideoFrameTransformerDelegate( RTPSenderVideo* sender, rtc::scoped_refptr frame_transformer, - uint32_t ssrc) + uint32_t ssrc, + TaskQueueBase* worker_queue) : sender_(sender), frame_transformer_(std::move(frame_transformer)), - ssrc_(ssrc) {} + ssrc_(ssrc), + worker_queue_(worker_queue) {} void RTPSenderVideoFrameTransformerDelegate::Init() { frame_transformer_->RegisterTransformedFrameSinkCallback( @@ -123,8 +125,14 @@ bool RTPSenderVideoFrameTransformerDelegate::TransformFrame( const RTPFragmentationHeader* fragmentation, RTPVideoHeader video_header, absl::optional expected_retransmission_time_ms) { - if (!encoder_queue_) - encoder_queue_ = TaskQueueBase::Current(); + if (!encoder_queue_) { + // Save the current task queue to post the transformed frame for sending + // once it is transformed. When there is no current task queue, i.e. + // encoding is done on an external thread (for example in the case of + // hardware encoders), use the worker queue instead. + TaskQueueBase* current = TaskQueueBase::Current(); + encoder_queue_ = current ? current : worker_queue_; + } frame_transformer_->Transform(std::make_unique( encoded_image, video_header, payload_type, codec_type, rtp_timestamp, fragmentation, expected_retransmission_time_ms, ssrc_)); diff --git a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.h b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.h index 29ac9e4e1c..bea5ba7b65 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.h +++ b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.h @@ -30,7 +30,8 @@ class RTPSenderVideoFrameTransformerDelegate : public TransformedFrameCallback { RTPSenderVideoFrameTransformerDelegate( RTPSenderVideo* sender, rtc::scoped_refptr frame_transformer, - uint32_t ssrc); + uint32_t ssrc, + TaskQueueBase* worker_queue); void Init(); @@ -69,6 +70,7 @@ class RTPSenderVideoFrameTransformerDelegate : public TransformedFrameCallback { rtc::scoped_refptr frame_transformer_; const uint32_t ssrc_; TaskQueueBase* encoder_queue_ = nullptr; + TaskQueueBase* worker_queue_; }; } // namespace webrtc From 528f23d0a169ca8f3ba1d856a3ae5e81cd0661aa Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Fri, 5 Jun 2020 12:36:32 +0200 Subject: [PATCH 07/12] Merge to M84: [VP9 SVC] Round spatial layers dimensions to ensure integer scaling factors are used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit // Skipping CQ, because bots are broken. The same bot passed yesterday. // Now two windows bots fail with "no executable found" or gn errors. TBR=srpang@webrtc.org (cherry picked from commit 09eb6e249d4e1f926dfa586bff9cc0de1c4cdefa) No-Try: True Bug: webrtc:11652 Change-Id: Id3642d607f62b72a567d521d9874b8588c2ce429 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176517 Reviewed-by: Erik Språng Commit-Queue: Ilya Nikolaevskiy Cr-Original-Commit-Position: refs/heads/master@{#31465} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176848 Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/branch-heads/4147@{#7} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- modules/video_coding/codecs/vp9/svc_config.cc | 5 ++++ .../video_coding/video_codec_initializer.cc | 8 +++++ video/video_stream_encoder.cc | 7 +++++ video/video_stream_encoder_unittest.cc | 30 +++++++++++++++++++ 4 files changed, 50 insertions(+) diff --git a/modules/video_coding/codecs/vp9/svc_config.cc b/modules/video_coding/codecs/vp9/svc_config.cc index e5d88bce21..b73661c48a 100644 --- a/modules/video_coding/codecs/vp9/svc_config.cc +++ b/modules/video_coding/codecs/vp9/svc_config.cc @@ -79,6 +79,11 @@ std::vector ConfigureSvcNormalVideo(size_t input_width, // First active layer must be configured. num_spatial_layers = std::max(num_spatial_layers, first_active_layer + 1); + // Ensure top layer is even enough. + int required_divisiblity = 1 << num_spatial_layers; + input_width = input_width - input_width % required_divisiblity; + input_height = input_height - input_height % required_divisiblity; + for (size_t sl_idx = first_active_layer; sl_idx < num_spatial_layers; ++sl_idx) { SpatialLayer spatial_layer = {0}; diff --git a/modules/video_coding/video_codec_initializer.cc b/modules/video_coding/video_codec_initializer.cc index e8665b9557..8671df71df 100644 --- a/modules/video_coding/video_codec_initializer.cc +++ b/modules/video_coding/video_codec_initializer.cc @@ -219,6 +219,14 @@ VideoCodec VideoCodecInitializer::VideoEncoderConfigToVideoCodec( video_codec.spatialLayers[i] = spatial_layers[i]; } + // The top spatial layer dimensions may not be equal to the input + // resolution because of the rounding or explicit configuration. + // This difference must be propagated to the stream configuration. + video_codec.width = spatial_layers.back().width; + video_codec.height = spatial_layers.back().height; + video_codec.simulcastStream[0].width = spatial_layers.back().width; + video_codec.simulcastStream[0].height = spatial_layers.back().height; + // Update layering settings. video_codec.VP9()->numberOfSpatialLayers = static_cast(spatial_layers.size()); diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 2684c4a61e..040e4789db 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -573,6 +573,13 @@ void VideoStreamEncoder::ReconfigureEncoder() { RTC_LOG(LS_ERROR) << "Failed to create encoder configuration."; } + if (encoder_config_.codec_type == kVideoCodecVP9) { + // Spatial layers configuration might impose some parity restrictions, + // thus some cropping might be needed. + crop_width_ = last_frame_info_->width - codec.width; + crop_height_ = last_frame_info_->height - codec.height; + } + char log_stream_buf[4 * 1024]; rtc::SimpleStringBuilder log_stream(log_stream_buf); log_stream << "ReconfigureEncoder:\n"; diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 158d7456c2..1c334fc3b3 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -5882,4 +5882,34 @@ TEST_F(VideoStreamEncoderTest, AutomaticAnimationDetection) { video_stream_encoder_->Stop(); } +TEST_F(VideoStreamEncoderTest, ConfiguresVp9SvcAtOddResolutions) { + const int kWidth = 720; // 540p adapted down. + const int kHeight = 405; + const int kNumFrames = 3; + // Works on screenshare mode. + ResetEncoder("VP9", /*num_streams=*/1, /*num_temporal_layers=*/1, + /*num_spatial_layers=*/2, /*screenshare=*/true); + + video_source_.set_adaptation_enabled(true); + + video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( + DataRate::BitsPerSec(kTargetBitrateBps), + DataRate::BitsPerSec(kTargetBitrateBps), + DataRate::BitsPerSec(kTargetBitrateBps), 0, 0, 0); + + VideoFrame frame = CreateFrame(1, kWidth, kHeight); + + // Pass enough frames with the full update to trigger animation detection. + for (int i = 0; i < kNumFrames; ++i) { + int64_t timestamp_ms = + fake_clock_.TimeNanos() / rtc::kNumNanosecsPerMillisec; + frame.set_ntp_time_ms(timestamp_ms); + frame.set_timestamp_us(timestamp_ms * 1000); + video_source_.IncomingCapturedFrame(frame); + WaitForEncodedFrame(timestamp_ms); + } + + video_stream_encoder_->Stop(); +} + } // namespace webrtc From a740523c6bb2630114937449cc97b844891cebaf Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Thu, 11 Jun 2020 14:01:56 +0200 Subject: [PATCH 08/12] Merge to M84: Allow HVGA Vp9 SVC to have 2 spatial layers and remove excessive rounding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (cherry picked from commit 2899b3bc3da018822ff34196b706d69c808fdb93) Bug: webrtc:11652 Change-Id: I8bfa91c3115d6ebb17beefbb2a5e51efbbd599e0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177000 Commit-Queue: Ilya Nikolaevskiy Reviewed-by: Erik Språng Cr-Original-Commit-Position: refs/heads/master@{#31502} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177105 Cr-Commit-Position: refs/branch-heads/4147@{#8} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- modules/video_coding/BUILD.gn | 1 + .../codecs/vp9/include/vp9_globals.h | 4 +-- modules/video_coding/codecs/vp9/svc_config.cc | 14 +++++++--- .../codecs/vp9/svc_config_unittest.cc | 26 +++++++++++++++++++ 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn index b1438392ae..41c952694d 100644 --- a/modules/video_coding/BUILD.gn +++ b/modules/video_coding/BUILD.gn @@ -505,6 +505,7 @@ rtc_library("webrtc_vp9_helpers") { "../../api/video_codecs:video_codecs_api", "../../common_video", "../../rtc_base:checks", + "../../rtc_base:logging", "../../rtc_base/experiments:stable_target_rate_experiment", "//third_party/abseil-cpp/absl/container:inlined_vector", ] diff --git a/modules/video_coding/codecs/vp9/include/vp9_globals.h b/modules/video_coding/codecs/vp9/include/vp9_globals.h index c6853127ac..6f9d09933f 100644 --- a/modules/video_coding/codecs/vp9/include/vp9_globals.h +++ b/modules/video_coding/codecs/vp9/include/vp9_globals.h @@ -30,8 +30,8 @@ const size_t kMaxVp9RefPics = 3; const size_t kMaxVp9FramesInGof = 0xFF; // 8 bits const size_t kMaxVp9NumberOfSpatialLayers = 8; -const size_t kMinVp9SpatialLayerWidth = 320; -const size_t kMinVp9SpatialLayerHeight = 180; +const size_t kMinVp9SpatialLayerWidth = 240; +const size_t kMinVp9SpatialLayerHeight = 135; enum TemporalStructureMode { kTemporalStructureMode1, // 1 temporal layer structure - i.e., IPPP... diff --git a/modules/video_coding/codecs/vp9/svc_config.cc b/modules/video_coding/codecs/vp9/svc_config.cc index b73661c48a..118d8a77c1 100644 --- a/modules/video_coding/codecs/vp9/svc_config.cc +++ b/modules/video_coding/codecs/vp9/svc_config.cc @@ -16,6 +16,7 @@ #include "modules/video_coding/codecs/vp9/include/vp9_globals.h" #include "rtc_base/checks.h" +#include "rtc_base/logging.h" namespace webrtc { @@ -74,13 +75,20 @@ std::vector ConfigureSvcNormalVideo(size_t input_width, const size_t num_layers_fit_vert = static_cast( std::floor(1 + std::max(0.0f, std::log2(1.0f * input_height / kMinVp9SpatialLayerHeight)))); - num_spatial_layers = - std::min({num_spatial_layers, num_layers_fit_horz, num_layers_fit_vert}); + const size_t limited_num_spatial_layers = + std::min(num_layers_fit_horz, num_layers_fit_vert); + if (limited_num_spatial_layers < num_spatial_layers) { + RTC_LOG(LS_WARNING) << "Reducing number of spatial layers from " + << num_spatial_layers << " to " + << limited_num_spatial_layers + << " due to low input resolution."; + num_spatial_layers = limited_num_spatial_layers; + } // First active layer must be configured. num_spatial_layers = std::max(num_spatial_layers, first_active_layer + 1); // Ensure top layer is even enough. - int required_divisiblity = 1 << num_spatial_layers; + int required_divisiblity = 1 << (num_spatial_layers - first_active_layer - 1); input_width = input_width - input_width % required_divisiblity; input_height = input_height - input_height % required_divisiblity; diff --git a/modules/video_coding/codecs/vp9/svc_config_unittest.cc b/modules/video_coding/codecs/vp9/svc_config_unittest.cc index abc67a22ff..1891628921 100644 --- a/modules/video_coding/codecs/vp9/svc_config_unittest.cc +++ b/modules/video_coding/codecs/vp9/svc_config_unittest.cc @@ -41,6 +41,32 @@ TEST(SvcConfig, AlwaysSendsAtLeastOneLayer) { EXPECT_EQ(spatial_layers.back().width, kMinVp9SpatialLayerWidth); } +TEST(SvcConfig, EnforcesMinimalRequiredParity) { + const size_t max_num_spatial_layers = 3; + const size_t kOddSize = 1023; + + std::vector spatial_layers = + GetSvcConfig(kOddSize, kOddSize, 30, + /*first_active_layer=*/1, max_num_spatial_layers, 1, false); + // Since there are 2 layers total (1, 2), divisiblity by 2 is required. + EXPECT_EQ(spatial_layers.back().width, kOddSize - 1); + EXPECT_EQ(spatial_layers.back().width, kOddSize - 1); + + spatial_layers = + GetSvcConfig(kOddSize, kOddSize, 30, + /*first_active_layer=*/0, max_num_spatial_layers, 1, false); + // Since there are 3 layers total (0, 1, 2), divisiblity by 4 is required. + EXPECT_EQ(spatial_layers.back().width, kOddSize - 3); + EXPECT_EQ(spatial_layers.back().width, kOddSize - 3); + + spatial_layers = + GetSvcConfig(kOddSize, kOddSize, 30, + /*first_active_layer=*/2, max_num_spatial_layers, 1, false); + // Since there is only 1 layer active (2), divisiblity by 1 is required. + EXPECT_EQ(spatial_layers.back().width, kOddSize); + EXPECT_EQ(spatial_layers.back().width, kOddSize); +} + TEST(SvcConfig, SkipsInactiveLayers) { const size_t num_spatial_layers = 4; const size_t first_active_layer = 2; From 963cc1ef1336b52ca27742beb28bfbc211ed54d0 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Tue, 16 Jun 2020 15:41:44 -0700 Subject: [PATCH 09/12] Merge to M84: Prevent pointer from being sent in the clear over SCTP. We were using the address of the SctpTransport object as the sconn_addr field in usrsctp, which is used to get access to the SctpTransport object in various callbacks. However, this address is sent in the clear in the SCTP cookie, which is undesirable. This change uses a monotonically increasing id instead, which is mapped back to a SctpTransport using a SctpTransportMap helper class. TBR=hta@webrtc.org Bug: chromium:1076703 Change-Id: I5c6a44801293e3b0aacd032f16f41802f4fecf6d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176422 Reviewed-by: Harald Alvestrand Reviewed-by: Tommi Commit-Queue: Taylor Cr-Original-Commit-Position: refs/heads/master@{#31449} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177330 Reviewed-by: Taylor Cr-Commit-Position: refs/branch-heads/4147@{#9} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- media/sctp/sctp_transport.cc | 87 +++++++++++++++++++++++++++++++----- media/sctp/sctp_transport.h | 5 +++ 2 files changed, 82 insertions(+), 10 deletions(-) diff --git a/media/sctp/sctp_transport.cc b/media/sctp/sctp_transport.cc index 40061a6048..1a6dc334e1 100644 --- a/media/sctp/sctp_transport.cc +++ b/media/sctp/sctp_transport.cc @@ -22,6 +22,7 @@ enum PreservedErrno { #include #include +#include #include "absl/algorithm/container.h" #include "absl/base/attributes.h" @@ -39,6 +40,7 @@ enum PreservedErrno { #include "rtc_base/logging.h" #include "rtc_base/numerics/safe_conversions.h" #include "rtc_base/string_utils.h" +#include "rtc_base/thread_annotations.h" #include "rtc_base/thread_checker.h" #include "rtc_base/trace_event.h" #include "usrsctplib/usrsctp.h" @@ -72,6 +74,59 @@ enum PayloadProtocolIdentifier { PPID_TEXT_LAST = 51 }; +// Maps SCTP transport ID to SctpTransport object, necessary in send threshold +// callback and outgoing packet callback. +// TODO(crbug.com/1076703): Remove once the underlying problem is fixed or +// workaround is provided in usrsctp. +class SctpTransportMap { + public: + SctpTransportMap() = default; + + // Assigns a new unused ID to the following transport. + uintptr_t Register(cricket::SctpTransport* transport) { + rtc::CritScope cs(&lock_); + // usrsctp_connect fails with a value of 0... + if (next_id_ == 0) { + ++next_id_; + } + // In case we've wrapped around and need to find an empty spot from a + // removed transport. Assumes we'll never be full. + while (map_.find(next_id_) != map_.end()) { + ++next_id_; + if (next_id_ == 0) { + ++next_id_; + } + }; + map_[next_id_] = transport; + return next_id_++; + } + + // Returns true if found. + bool Deregister(uintptr_t id) { + rtc::CritScope cs(&lock_); + return map_.erase(id) > 0; + } + + cricket::SctpTransport* Retrieve(uintptr_t id) const { + rtc::CritScope cs(&lock_); + auto it = map_.find(id); + if (it == map_.end()) { + return nullptr; + } + return it->second; + } + + private: + rtc::CriticalSection lock_; + + uintptr_t next_id_ RTC_GUARDED_BY(lock_) = 0; + std::unordered_map map_ + RTC_GUARDED_BY(lock_); +}; + +// Should only be modified by UsrSctpWrapper. +ABSL_CONST_INIT SctpTransportMap* g_transport_map_ = nullptr; + // Helper for logging SCTP messages. #if defined(__GNUC__) __attribute__((__format__(__printf__, 1, 2))) @@ -242,9 +297,12 @@ class SctpTransport::UsrSctpWrapper { // Set the number of default outgoing streams. This is the number we'll // send in the SCTP INIT message. usrsctp_sysctl_set_sctp_nr_outgoing_streams_default(kMaxSctpStreams); + + g_transport_map_ = new SctpTransportMap(); } static void UninitializeUsrSctp() { + delete g_transport_map_; RTC_LOG(LS_INFO) << __FUNCTION__; // usrsctp_finish() may fail if it's called too soon after the transports // are @@ -282,7 +340,14 @@ class SctpTransport::UsrSctpWrapper { size_t length, uint8_t tos, uint8_t set_df) { - SctpTransport* transport = static_cast(addr); + SctpTransport* transport = + g_transport_map_->Retrieve(reinterpret_cast(addr)); + if (!transport) { + RTC_LOG(LS_ERROR) + << "OnSctpOutboundPacket: Failed to get transport for socket ID " + << addr; + return EINVAL; + } RTC_LOG(LS_VERBOSE) << "global OnSctpOutboundPacket():" "addr: " << addr << "; length: " << length @@ -392,14 +457,14 @@ class SctpTransport::UsrSctpWrapper { return nullptr; } // usrsctp_getladdrs() returns the addresses bound to this socket, which - // contains the SctpTransport* as sconn_addr. Read the pointer, + // contains the SctpTransport id as sconn_addr. Read the id, // then free the list of addresses once we have the pointer. We only open // AF_CONN sockets, and they should all have the sconn_addr set to the - // pointer that created them, so [0] is as good as any other. + // id of the transport that created them, so [0] is as good as any other. struct sockaddr_conn* sconn = reinterpret_cast(&addrs[0]); - SctpTransport* transport = - reinterpret_cast(sconn->sconn_addr); + SctpTransport* transport = g_transport_map_->Retrieve( + reinterpret_cast(sconn->sconn_addr)); usrsctp_freeladdrs(addrs); return transport; @@ -779,9 +844,10 @@ bool SctpTransport::OpenSctpSocket() { UsrSctpWrapper::DecrementUsrSctpUsageCount(); return false; } - // Register this class as an address for usrsctp. This is used by SCTP to + id_ = g_transport_map_->Register(this); + // Register our id as an address for usrsctp. This is used by SCTP to // direct the packets received (by the created socket) to this class. - usrsctp_register_address(this); + usrsctp_register_address(reinterpret_cast(id_)); return true; } @@ -872,7 +938,8 @@ void SctpTransport::CloseSctpSocket() { // discarded instead of being sent. usrsctp_close(sock_); sock_ = nullptr; - usrsctp_deregister_address(this); + usrsctp_deregister_address(reinterpret_cast(id_)); + RTC_CHECK(g_transport_map_->Deregister(id_)); UsrSctpWrapper::DecrementUsrSctpUsageCount(); ready_to_send_data_ = false; } @@ -1003,7 +1070,7 @@ void SctpTransport::OnPacketRead(rtc::PacketTransportInternal* transport, // will be will be given to the global OnSctpInboundData, and then, // marshalled by the AsyncInvoker. VerboseLogPacket(data, len, SCTP_DUMP_INBOUND); - usrsctp_conninput(this, data, len, 0); + usrsctp_conninput(reinterpret_cast(id_), data, len, 0); } else { // TODO(ldixon): Consider caching the packet for very slightly better // reliability. @@ -1033,7 +1100,7 @@ sockaddr_conn SctpTransport::GetSctpSockAddr(int port) { #endif // Note: conversion from int to uint16_t happens here. sconn.sconn_port = rtc::HostToNetwork16(port); - sconn.sconn_addr = this; + sconn.sconn_addr = reinterpret_cast(id_); return sconn; } diff --git a/media/sctp/sctp_transport.h b/media/sctp/sctp_transport.h index d346cfc71f..758503b509 100644 --- a/media/sctp/sctp_transport.h +++ b/media/sctp/sctp_transport.h @@ -13,6 +13,7 @@ #include +#include #include #include #include @@ -267,6 +268,10 @@ class SctpTransport : public SctpTransportInternal, absl::optional max_outbound_streams_; absl::optional max_inbound_streams_; + // Used for associating this transport with the underlying sctp socket in + // various callbacks. + uintptr_t id_ = 0; + RTC_DISALLOW_COPY_AND_ASSIGN(SctpTransport); }; From 758c388d3fedac4a474b48a240af83dd3f6a5fc5 Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Thu, 18 Jun 2020 19:16:53 +0200 Subject: [PATCH 10/12] Merge to M84: Fix vp9 svc singlecast mode and enable quality scaler for vp9 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1) Fix several typos and small mistakes which could lead to crashes 2) Adjust bitrates if leading layers are disabled 3) Wire up webrtc quality scaler (cherry picked from commit 7a82467d0db0d61f466a1da54b94f6a136726a3c) Bug: webrtc:11319 Change-Id: I16e52bdb1c315d64906288e4f2be55fe698d5ceb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177525 Commit-Queue: Ilya Nikolaevskiy Reviewed-by: Erik Språng Cr-Original-Commit-Position: refs/heads/master@{#31546} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177660 Cr-Commit-Position: refs/branch-heads/4147@{#10} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- modules/video_coding/codecs/vp9/svc_config.cc | 13 ++++++ .../codecs/vp9/svc_rate_allocator.cc | 3 +- modules/video_coding/codecs/vp9/vp9_impl.cc | 45 +++++++++++++++++-- modules/video_coding/codecs/vp9/vp9_impl.h | 9 ++++ .../video_coding/video_codec_initializer.cc | 6 ++- video/quality_scaling_tests.cc | 3 +- video/video_stream_encoder.cc | 3 +- 7 files changed, 74 insertions(+), 8 deletions(-) diff --git a/modules/video_coding/codecs/vp9/svc_config.cc b/modules/video_coding/codecs/vp9/svc_config.cc index 118d8a77c1..cc7743ad25 100644 --- a/modules/video_coding/codecs/vp9/svc_config.cc +++ b/modules/video_coding/codecs/vp9/svc_config.cc @@ -121,6 +121,19 @@ std::vector ConfigureSvcNormalVideo(size_t input_width, spatial_layers.push_back(spatial_layer); } + // A workaround for sitiation when single HD layer is left with minBitrate + // about 500kbps. This would mean that there will always be at least 500kbps + // allocated to video regardless of how low is the actual BWE. + // Also, boost maxBitrate for the first layer to account for lost ability to + // predict from previous layers. + if (first_active_layer > 0) { + spatial_layers[0].minBitrate = kMinVp9SvcBitrateKbps; + // TODO(ilnik): tune this value or come up with a different formula to + // ensure that all singlecast configurations look good and not too much + // bitrate is added. + spatial_layers[0].maxBitrate *= 1.1; + } + return spatial_layers; } diff --git a/modules/video_coding/codecs/vp9/svc_rate_allocator.cc b/modules/video_coding/codecs/vp9/svc_rate_allocator.cc index cc9a0d8997..25bca63c0e 100644 --- a/modules/video_coding/codecs/vp9/svc_rate_allocator.cc +++ b/modules/video_coding/codecs/vp9/svc_rate_allocator.cc @@ -140,7 +140,8 @@ DataRate FindLayerTogglingThreshold(const VideoCodec& codec, } } upper_bound += DataRate::KilobitsPerSec( - codec.spatialLayers[num_active_layers - 1].minBitrate); + codec.spatialLayers[first_active_layer + num_active_layers - 1] + .minBitrate); // Do a binary search until upper and lower bound is the highest bitrate for // |num_active_layers| - 1 layers and lowest bitrate for |num_active_layers| diff --git a/modules/video_coding/codecs/vp9/vp9_impl.cc b/modules/video_coding/codecs/vp9/vp9_impl.cc index 46f72b6e02..028d3ab8f7 100644 --- a/modules/video_coding/codecs/vp9/vp9_impl.cc +++ b/modules/video_coding/codecs/vp9/vp9_impl.cc @@ -53,6 +53,15 @@ const int kMaxAllowedPidDiff = 30; constexpr double kLowRateFactor = 1.0; constexpr double kHighRateFactor = 2.0; +// TODO(ilink): Tune these thresholds further. +// Selected using ConverenceMotion_1280_720_50.yuv clip. +// No toggling observed on any link capacity from 100-2000kbps. +// HD was reached consistently when link capacity was 1500kbps. +// Set resolutions are a bit more conservative than svc_config.cc sets, e.g. +// for 300kbps resolution converged to 270p instead of 360p. +constexpr int kLowVp9QpThreshold = 149; +constexpr int kHighVp9QpThreshold = 205; + // These settings correspond to the settings in vpx_codec_enc_cfg. struct Vp9RateSettings { uint32_t rc_undershoot_pct; @@ -249,6 +258,8 @@ VP9EncoderImpl::VP9EncoderImpl(const cricket::VideoCodec& codec) "WebRTC-VP9VariableFramerateScreenshare")), variable_framerate_controller_( variable_framerate_experiment_.framerate_limit), + quality_scaler_experiment_( + ParseQualityScalerConfig("WebRTC-VP9QualityScaler")), num_steady_state_frames_(0), config_changed_(true) { codec_ = {}; @@ -399,7 +410,6 @@ bool VP9EncoderImpl::SetSvcRates( expect_no_more_active_layers = seen_active_layer; } } - RTC_DCHECK_GT(num_active_spatial_layers_, 0); if (higher_layers_enabled && !force_key_frame_) { // Prohibit drop of all layers for the next frame, so newly enabled @@ -562,7 +572,13 @@ int VP9EncoderImpl::InitEncode(const VideoCodec* inst, // put some key-frames at will even in VPX_KF_DISABLED kf_mode. config_->kf_max_dist = inst->VP9().keyFrameInterval; config_->kf_min_dist = config_->kf_max_dist; - config_->rc_resize_allowed = inst->VP9().automaticResizeOn ? 1 : 0; + if (quality_scaler_experiment_.enabled) { + // In that experiment webrtc wide quality scaler is used instead of libvpx + // internal scaler. + config_->rc_resize_allowed = 0; + } else { + config_->rc_resize_allowed = inst->VP9().automaticResizeOn ? 1 : 0; + } // Determine number of threads based on the image size and #cores. config_->g_threads = NumberOfThreads(config_->g_w, config_->g_h, settings.number_of_cores); @@ -1554,7 +1570,12 @@ VideoEncoder::EncoderInfo VP9EncoderImpl::GetEncoderInfo() const { EncoderInfo info; info.supports_native_handle = false; info.implementation_name = "libvpx"; - info.scaling_settings = VideoEncoder::ScalingSettings::kOff; + if (quality_scaler_experiment_.enabled) { + info.scaling_settings = VideoEncoder::ScalingSettings( + quality_scaler_experiment_.low_qp, quality_scaler_experiment_.high_qp); + } else { + info.scaling_settings = VideoEncoder::ScalingSettings::kOff; + } info.has_trusted_rate_controller = trusted_rate_controller_; info.is_hardware_accelerated = false; info.has_internal_source = false; @@ -1627,6 +1648,24 @@ VP9EncoderImpl::ParseVariableFramerateConfig(std::string group_name) { return config; } +// static +VP9EncoderImpl::QualityScalerExperiment +VP9EncoderImpl::ParseQualityScalerConfig(std::string group_name) { + FieldTrialFlag disabled = FieldTrialFlag("Disabled"); + FieldTrialParameter low_qp("low_qp", kLowVp9QpThreshold); + FieldTrialParameter high_qp("hihg_qp", kHighVp9QpThreshold); + ParseFieldTrial({&disabled, &low_qp, &high_qp}, + field_trial::FindFullName(group_name)); + QualityScalerExperiment config; + config.enabled = !disabled.Get(); + RTC_LOG(LS_INFO) << "Webrtc quality scaler for vp9 is " + << (config.enabled ? "enabled." : "disabled"); + config.low_qp = low_qp.Get(); + config.high_qp = high_qp.Get(); + + return config; +} + VP9DecoderImpl::VP9DecoderImpl() : decode_complete_callback_(nullptr), inited_(false), diff --git a/modules/video_coding/codecs/vp9/vp9_impl.h b/modules/video_coding/codecs/vp9/vp9_impl.h index 2126044dcc..f6d8318d7d 100644 --- a/modules/video_coding/codecs/vp9/vp9_impl.h +++ b/modules/video_coding/codecs/vp9/vp9_impl.h @@ -175,6 +175,15 @@ class VP9EncoderImpl : public VP9Encoder { static VariableFramerateExperiment ParseVariableFramerateConfig( std::string group_name); FramerateController variable_framerate_controller_; + + const struct QualityScalerExperiment { + int low_qp; + int high_qp; + bool enabled; + } quality_scaler_experiment_; + static QualityScalerExperiment ParseQualityScalerConfig( + std::string group_name); + int num_steady_state_frames_; // Only set config when this flag is set. bool config_changed_; diff --git a/modules/video_coding/video_codec_initializer.cc b/modules/video_coding/video_codec_initializer.cc index 8671df71df..7f36f99f89 100644 --- a/modules/video_coding/video_codec_initializer.cc +++ b/modules/video_coding/video_codec_initializer.cc @@ -75,7 +75,9 @@ VideoCodec VideoCodecInitializer::VideoEncoderConfigToVideoCodec( static_cast(streams.size()); video_codec.minBitrate = streams[0].min_bitrate_bps / 1000; bool codec_active = false; - for (const VideoStream& stream : streams) { + // Active configuration might not be fully copied to |streams| for SVC yet. + // Therefore the |config| is checked here. + for (const VideoStream& stream : config.simulcast_layers) { if (stream.active) { codec_active = true; break; @@ -205,7 +207,7 @@ VideoCodec VideoCodecInitializer::VideoEncoderConfigToVideoCodec( spatial_layers.back().maxBitrate = video_codec.maxBitrate; } - for (size_t spatial_idx = 0; + for (size_t spatial_idx = first_active_layer; spatial_idx < config.simulcast_layers.size() && spatial_idx < spatial_layers.size(); ++spatial_idx) { diff --git a/video/quality_scaling_tests.cc b/video/quality_scaling_tests.cc index 19b9e8c36c..65a23dbbcc 100644 --- a/video/quality_scaling_tests.cc +++ b/video/quality_scaling_tests.cc @@ -233,7 +233,8 @@ TEST_F(QualityScalingTest, NoAdaptDownForLowStartBitrateWithScalingOff) { TEST_F(QualityScalingTest, NoAdaptDownForHighQp_Vp9) { // VP9 QP thresholds, low:1, high:1 -> high QP. - test::ScopedFieldTrials field_trials(kPrefix + "0,0,1,1,0,0" + kEnd); + test::ScopedFieldTrials field_trials(kPrefix + "0,0,1,1,0,0" + kEnd + + "WebRTC-VP9QualityScaler/Disabled/"); // QualityScaler always disabled. const bool kAutomaticResize = true; diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 040e4789db..92ab5fc5c3 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -1719,7 +1719,8 @@ bool VideoStreamEncoder::DropDueToSize(uint32_t pixel_count) const { bool simulcast_or_svc = (send_codec_.codecType == VideoCodecType::kVideoCodecVP9 && send_codec_.VP9().numberOfSpatialLayers > 1) || - send_codec_.numberOfSimulcastStreams > 1; + send_codec_.numberOfSimulcastStreams > 1 || + encoder_config_.simulcast_layers.size() > 1; if (simulcast_or_svc || !stream_resource_manager_.DropInitialFrames() || !encoder_target_bitrate_bps_.has_value()) { From d25c2ac74afc25f65d111771dbfabd6db25d2498 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Tue, 21 Jul 2020 13:57:58 -0700 Subject: [PATCH 11/12] Merge to M84: Check for null before accessing SctpTransport map. (cherry picked from commit c7c412a36cbea0812122985872d1fcb34f688f80) TBR=hta@webrtc.org Bug: chromium:1104061 Change-Id: I52d44ff1603341777a873e747c625665bc11bfa5 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/179161 Commit-Queue: Taylor Reviewed-by: Harald Alvestrand Cr-Original-Commit-Position: refs/heads/master@{#31720} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/179900 Reviewed-by: Taylor Cr-Commit-Position: refs/branch-heads/4147@{#11} Cr-Branched-From: 2b7d96959916306cb267899a219cbd20ec24b841-refs/heads/master@{#31262} --- media/sctp/sctp_transport.cc | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/media/sctp/sctp_transport.cc b/media/sctp/sctp_transport.cc index 1a6dc334e1..ad68c37ac9 100644 --- a/media/sctp/sctp_transport.cc +++ b/media/sctp/sctp_transport.cc @@ -302,18 +302,21 @@ class SctpTransport::UsrSctpWrapper { } static void UninitializeUsrSctp() { - delete g_transport_map_; RTC_LOG(LS_INFO) << __FUNCTION__; // usrsctp_finish() may fail if it's called too soon after the transports // are // closed. Wait and try again until it succeeds for up to 3 seconds. for (size_t i = 0; i < 300; ++i) { if (usrsctp_finish() == 0) { + delete g_transport_map_; + g_transport_map_ = nullptr; return; } rtc::Thread::SleepMs(10); } + delete g_transport_map_; + g_transport_map_ = nullptr; RTC_LOG(LS_ERROR) << "Failed to shutdown usrsctp."; } @@ -340,6 +343,11 @@ class SctpTransport::UsrSctpWrapper { size_t length, uint8_t tos, uint8_t set_df) { + if (!g_transport_map_) { + RTC_LOG(LS_ERROR) + << "OnSctpOutboundPacket called after usrsctp uninitialized?"; + return EINVAL; + } SctpTransport* transport = g_transport_map_->Retrieve(reinterpret_cast(addr)); if (!transport) { @@ -463,6 +471,12 @@ class SctpTransport::UsrSctpWrapper { // id of the transport that created them, so [0] is as good as any other. struct sockaddr_conn* sconn = reinterpret_cast(&addrs[0]); + if (!g_transport_map_) { + RTC_LOG(LS_ERROR) + << "GetTransportFromSocket called after usrsctp uninitialized?"; + usrsctp_freeladdrs(addrs); + return nullptr; + } SctpTransport* transport = g_transport_map_->Retrieve( reinterpret_cast(sconn->sconn_addr)); usrsctp_freeladdrs(addrs); From fcf847fa8d9a0b9d6f287455cbc6857ded983a68 Mon Sep 17 00:00:00 2001 From: Minhyuk Kim Date: Wed, 26 Aug 2020 10:15:17 +0900 Subject: [PATCH 12/12] Cherry pick changes --- .gitignore | 1 + README.md | 8 ++++++++ sdk/objc/api/peerconnection/RTCPeerConnection.h | 1 + tools_webrtc/ios/build_ios_libs.py | 6 +++--- 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 8c7582c3a2..c43e108f51 100644 --- a/.gitignore +++ b/.gitignore @@ -71,3 +71,4 @@ /xcodebuild /.vscode !webrtc/* +out_ios_libs diff --git a/README.md b/README.md index e91fb16ce7..23f82d3f4b 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,11 @@ +This repository is a private branch of [Google's WebRTC Source](https://chromium.googlesource.com/external/webrtc.git). + +This repository is used to compile [SendBirdWebRTC](https://github.com/sendbird/sendbird-webrtc-ios), and keep track of SendBird's changes. + +In order to use this repository as a reflection of Google's Public WebRTC source, refer to this [medium guide](https://medium.com/@bilalbayasut/github-how-to-make-a-fork-of-public-repository-private-6ee8cacaf9d3) to fork public repository into this private repository. + +--- + **WebRTC is a free, open software project** that provides browsers and mobile applications with Real-Time Communications (RTC) capabilities via simple APIs. The WebRTC components have been optimized to best serve this purpose. diff --git a/sdk/objc/api/peerconnection/RTCPeerConnection.h b/sdk/objc/api/peerconnection/RTCPeerConnection.h index bb8d87bc2d..38a5dc2a4d 100644 --- a/sdk/objc/api/peerconnection/RTCPeerConnection.h +++ b/sdk/objc/api/peerconnection/RTCPeerConnection.h @@ -343,6 +343,7 @@ RTC_OBJC_EXPORT typedef void (^RTCStatisticsCompletionHandler)(RTC_OBJC_TYPE(RTCStatisticsReport) *); +RTC_OBJC_EXPORT @interface RTC_OBJC_TYPE (RTCPeerConnection) (Stats) diff --git a/tools_webrtc/ios/build_ios_libs.py b/tools_webrtc/ios/build_ios_libs.py index b0d28c0151..538bbad2e8 100755 --- a/tools_webrtc/ios/build_ios_libs.py +++ b/tools_webrtc/ios/build_ios_libs.py @@ -33,8 +33,8 @@ SDK_FRAMEWORK_NAME = 'WebRTC.framework' DEFAULT_ARCHS = ENABLED_ARCHS = ['arm64', 'arm', 'x64', 'x86'] -IOS_DEPLOYMENT_TARGET = '10.0' -LIBVPX_BUILD_VP9 = False +IOS_DEPLOYMENT_TARGET = '9.0' +LIBVPX_BUILD_VP9 = True sys.path.append(os.path.join(SCRIPT_DIR, '..', 'libs')) from generate_licenses import LicenseBuilder @@ -59,7 +59,7 @@ def _ParseArgs(): 'If specified together with -c, deletes the dir.') parser.add_argument('-r', '--revision', type=int, default=0, help='Specifies a revision number to embed if building the framework.') - parser.add_argument('-e', '--bitcode', action='store_true', default=False, + parser.add_argument('-e', '--bitcode', action='store_true', default=True, help='Compile with bitcode.') parser.add_argument('--verbose', action='store_true', default=False, help='Debug logging.')