From a7b691499b7b93de09840c088021f3fa6317b547 Mon Sep 17 00:00:00 2001 From: Evan Shrubsole Date: Wed, 22 Mar 2023 12:36:48 +0000 Subject: [PATCH] Always post RemovePacketsForSsrc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If during shutdown the pacer has packets enqueued during pause these packets will be posted to the pacer after the worker thread is free - after the ssrcs should have been cleared. This fixes flakes in picutre_id_tests. Bug: webrtc:14985 Change-Id: Ib5547a501670fc145543df32fdc43bbc6596375f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/297401 Reviewed-by: Per Kjellander Reviewed-by: Erik Språng Commit-Queue: Evan Shrubsole Cr-Commit-Position: refs/heads/main@{#39640} --- modules/pacing/BUILD.gn | 1 + modules/pacing/task_queue_paced_sender.cc | 11 +++-- .../task_queue_paced_sender_unittest.cc | 47 ++++++++++++++++++- video/picture_id_tests.cc | 10 ++-- 4 files changed, 56 insertions(+), 13 deletions(-) diff --git a/modules/pacing/BUILD.gn b/modules/pacing/BUILD.gn index 3fdac71a0c..fb36f92761 100644 --- a/modules/pacing/BUILD.gn +++ b/modules/pacing/BUILD.gn @@ -98,6 +98,7 @@ if (rtc_include_tests) { "../../api/task_queue:task_queue", "../../api/transport:network_control", "../../api/units:data_rate", + "../../api/units:data_size", "../../api/units:time_delta", "../../api/units:timestamp", "../../rtc_base:checks", diff --git a/modules/pacing/task_queue_paced_sender.cc b/modules/pacing/task_queue_paced_sender.cc index bfe97922ac..2747760eff 100644 --- a/modules/pacing/task_queue_paced_sender.cc +++ b/modules/pacing/task_queue_paced_sender.cc @@ -155,11 +155,12 @@ void TaskQueuePacedSender::EnqueuePackets( } void TaskQueuePacedSender::RemovePacketsForSsrc(uint32_t ssrc) { - task_queue_.RunOrPost([this, ssrc]() { - RTC_DCHECK_RUN_ON(&task_queue_); - pacing_controller_.RemovePacketsForSsrc(ssrc); - MaybeProcessPackets(Timestamp::MinusInfinity()); - }); + task_queue_.TaskQueueForPost()->PostTask( + task_queue_.MaybeSafeTask(safety_.flag(), [this, ssrc] { + RTC_DCHECK_RUN_ON(&task_queue_); + pacing_controller_.RemovePacketsForSsrc(ssrc); + MaybeProcessPackets(Timestamp::MinusInfinity()); + })); } void TaskQueuePacedSender::SetAccountForAudioPackets(bool account_for_audio) { diff --git a/modules/pacing/task_queue_paced_sender_unittest.cc b/modules/pacing/task_queue_paced_sender_unittest.cc index 55ec1445c9..a8a7b65ef4 100644 --- a/modules/pacing/task_queue_paced_sender_unittest.cc +++ b/modules/pacing/task_queue_paced_sender_unittest.cc @@ -18,11 +18,14 @@ #include #include -#include "absl/functional/any_invocable.h" #include "api/task_queue/task_queue_base.h" +#include "api/task_queue/task_queue_factory.h" #include "api/transport/network_types.h" #include "api/units/data_rate.h" +#include "api/units/data_size.h" +#include "api/units/time_delta.h" #include "modules/pacing/packet_router.h" +#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "test/gmock.h" #include "test/gtest.h" #include "test/scoped_key_value_config.h" @@ -667,6 +670,48 @@ TEST_P(TaskQueuePacedSenderTest, ProbingStopDuringSendLoop) { time_controller.AdvanceTime(kPacketsPacedTime + TimeDelta::Millis(1)); } +TEST_P(TaskQueuePacedSenderTest, PostedPacketsNotSendFromRemovePacketsForSsrc) { + static constexpr Timestamp kStartTime = Timestamp::Millis(1234); + GlobalSimulatedTimeController time_controller(kStartTime); + ScopedKeyValueConfig trials(GetParam()); + MockPacketRouter packet_router; + TaskQueuePacedSender pacer(time_controller.GetClock(), &packet_router, trials, + time_controller.GetTaskQueueFactory(), + PacingController::kMinSleepTime, + TaskQueuePacedSender::kNoPacketHoldback); + + static constexpr DataRate kPacingRate = + DataRate::BytesPerSec(kDefaultPacketSize * 10); + pacer.SetPacingRates(kPacingRate, DataRate::Zero()); + pacer.EnsureStarted(); + + auto encoder_queue = time_controller.GetTaskQueueFactory()->CreateTaskQueue( + "encoder_queue", TaskQueueFactory::Priority::HIGH); + + EXPECT_CALL(packet_router, SendPacket).Times(5); + encoder_queue->PostTask([&pacer] { + pacer.EnqueuePackets(GeneratePackets(RtpPacketMediaType::kVideo, 6)); + }); + + time_controller.AdvanceTime(TimeDelta::Millis(400)); + // 1 packet left. + EXPECT_EQ(pacer.OldestPacketWaitTime(), TimeDelta::Millis(400)); + EXPECT_EQ(pacer.FirstSentPacketTime(), kStartTime); + + // Enqueue packets while removing ssrcs should not send any more packets. + encoder_queue->PostTask( + [&pacer, worker_thread = time_controller.GetMainThread()] { + worker_thread->PostTask( + [&pacer] { pacer.RemovePacketsForSsrc(kVideoSsrc); }); + pacer.EnqueuePackets(GeneratePackets(RtpPacketMediaType::kVideo, 5)); + }); + time_controller.AdvanceTime(TimeDelta::Seconds(1)); + EXPECT_EQ(pacer.OldestPacketWaitTime(), TimeDelta::Zero()); + EXPECT_EQ(pacer.FirstSentPacketTime(), kStartTime); + EXPECT_EQ(pacer.QueueSizeData(), DataSize::Zero()); + EXPECT_EQ(pacer.ExpectedQueueTime(), TimeDelta::Zero()); +} + TEST_P(TaskQueuePacedSenderTest, Stats) { static constexpr Timestamp kStartTime = Timestamp::Millis(1234); GlobalSimulatedTimeController time_controller(kStartTime); diff --git a/video/picture_id_tests.cc b/video/picture_id_tests.cc index 71b1c34424..06491b924a 100644 --- a/video/picture_id_tests.cc +++ b/video/picture_id_tests.cc @@ -367,8 +367,7 @@ TEST_P(PictureIdTest, ContinuousAfterReconfigureVp8) { TestPictureIdContinuousAfterReconfigure({1, 3, 3, 1, 1}); } -// TODO(bugs.webrtc.org/14985): Investigate and reenable. -TEST_P(PictureIdTest, DISABLED_IncreasingAfterRecreateStreamVp8) { +TEST_P(PictureIdTest, IncreasingAfterRecreateStreamVp8) { test::FunctionVideoEncoderFactory encoder_factory( []() { return VP8Encoder::Create(); }); SetupEncoder(&encoder_factory, "VP8"); @@ -395,9 +394,7 @@ TEST_P(PictureIdTest, ContinuousAfterReconfigureSimulcastEncoderAdapter) { TestPictureIdContinuousAfterReconfigure({1, 3, 3, 1, 1}); } -// TODO(bugs.webrtc.org/14985): Investigate and reenable. -TEST_P(PictureIdTest, - DISABLED_IncreasingAfterRecreateStreamSimulcastEncoderAdapter) { +TEST_P(PictureIdTest, IncreasingAfterRecreateStreamSimulcastEncoderAdapter) { InternalEncoderFactory internal_encoder_factory; test::FunctionVideoEncoderFactory encoder_factory( [&internal_encoder_factory]() { @@ -421,8 +418,7 @@ TEST_P(PictureIdTest, ContinuousAfterStreamCountChangeSimulcastEncoderAdapter) { TestPictureIdContinuousAfterReconfigure({3, 1, 3}); } -// TODO(bugs.webrtc.org/14985): Investigate and reenable. -TEST_P(PictureIdTest, DISABLED_IncreasingAfterRecreateStreamVp9) { +TEST_P(PictureIdTest, IncreasingAfterRecreateStreamVp9) { test::FunctionVideoEncoderFactory encoder_factory( []() { return VP9Encoder::Create(); }); SetupEncoder(&encoder_factory, "VP9");