From 1a6a4f3d6270d5b6adb96ba5ff6dd32c65ff7a45 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 5 Sep 2018 12:24:07 +0200 Subject: [PATCH] Replace WeakPtr with CancelablePeriodicTask in RtcpTransceiverImpl This allow to destroy the RtcpTransceiverImpl off the task queue with assumption it is destroyed after task queue. i.e. it allows to post destruction of RtcpTransceiverImpl to the TaskQueue without waiting. BUG: webrtc:8239 Change-Id: I4bea7a6d2edc97061ebd00f2f275c1ab827bc3c5 Reviewed-on: https://webrtc-review.googlesource.com/97160 Commit-Queue: Danil Chapovalov Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/master@{#24574} --- modules/rtp_rtcp/BUILD.gn | 1 + .../rtp_rtcp/source/rtcp_transceiver_impl.cc | 51 ++++++++----------- .../rtp_rtcp/source/rtcp_transceiver_impl.h | 4 +- .../source/rtcp_transceiver_impl_unittest.cc | 36 ++++++++++++- 4 files changed, 58 insertions(+), 34 deletions(-) diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn index 4f36a2ed85..f78b4efc5e 100644 --- a/modules/rtp_rtcp/BUILD.gn +++ b/modules/rtp_rtcp/BUILD.gn @@ -253,6 +253,7 @@ rtc_source_set("rtcp_transceiver") { "../../api/video:video_bitrate_allocation", "../../rtc_base:checks", "../../rtc_base:rtc_base_approved", + "../../rtc_base:rtc_cancelable_task", "../../rtc_base:rtc_task_queue", "../../rtc_base:weak_ptr", "../../system_wrappers", diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc index a4da63a10f..2c6c3acc39 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc @@ -29,6 +29,7 @@ #include "modules/rtp_rtcp/source/rtcp_packet/sdes.h" #include "modules/rtp_rtcp/source/rtcp_packet/sender_report.h" #include "modules/rtp_rtcp/source/time_util.h" +#include "rtc_base/cancelable_periodic_task.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" #include "rtc_base/task_queue.h" @@ -87,15 +88,19 @@ class RtcpTransceiverImpl::PacketSender { }; RtcpTransceiverImpl::RtcpTransceiverImpl(const RtcpTransceiverConfig& config) - : config_(config), - ready_to_send_(config.initial_ready_to_send), - ptr_factory_(this) { + : config_(config), ready_to_send_(config.initial_ready_to_send) { RTC_CHECK(config_.Validate()); if (ready_to_send_ && config_.schedule_periodic_compound_packets) SchedulePeriodicCompoundPackets(config_.initial_report_delay_ms); } -RtcpTransceiverImpl::~RtcpTransceiverImpl() = default; +RtcpTransceiverImpl::~RtcpTransceiverImpl() { + // If RtcpTransceiverImpl is destroyed off task queue, assume it is destroyed + // after TaskQueue. In that case there is no need to Cancel periodic task. + if (config_.task_queue == rtc::TaskQueue::Current()) { + periodic_task_handle_.Cancel(); + } +} void RtcpTransceiverImpl::AddMediaReceiverRtcpObserver( uint32_t remote_ssrc, @@ -120,8 +125,8 @@ void RtcpTransceiverImpl::RemoveMediaReceiverRtcpObserver( void RtcpTransceiverImpl::SetReadyToSend(bool ready) { if (config_.schedule_periodic_compound_packets) { - if (ready_to_send_ && !ready) // Stop existent send task. - ptr_factory_.InvalidateWeakPtrs(); + if (ready_to_send_ && !ready) + periodic_task_handle_.Cancel(); if (!ready_to_send_ && ready) // Restart periodic sending. SchedulePeriodicCompoundPackets(config_.report_period_ms / 2); @@ -318,36 +323,20 @@ void RtcpTransceiverImpl::HandleTargetBitrate( void RtcpTransceiverImpl::ReschedulePeriodicCompoundPackets() { if (!config_.schedule_periodic_compound_packets) return; - // Stop existent send task. - ptr_factory_.InvalidateWeakPtrs(); + periodic_task_handle_.Cancel(); + RTC_DCHECK(ready_to_send_); SchedulePeriodicCompoundPackets(config_.report_period_ms); } void RtcpTransceiverImpl::SchedulePeriodicCompoundPackets(int64_t delay_ms) { - class SendPeriodicCompoundPacketTask : public rtc::QueuedTask { - public: - SendPeriodicCompoundPacketTask(rtc::TaskQueue* task_queue, - rtc::WeakPtr ptr) - : task_queue_(task_queue), ptr_(std::move(ptr)) {} - bool Run() override { - RTC_DCHECK(task_queue_->IsCurrent()); - if (!ptr_) - return true; - ptr_->SendPeriodicCompoundPacket(); - task_queue_->PostDelayedTask(absl::WrapUnique(this), - ptr_->config_.report_period_ms); - return false; - } + auto task = rtc::CreateCancelablePeriodicTask([this] { + RTC_DCHECK(config_.schedule_periodic_compound_packets); + RTC_DCHECK(ready_to_send_); + SendPeriodicCompoundPacket(); + return config_.report_period_ms; + }); + periodic_task_handle_ = task->GetCancellationHandle(); - private: - rtc::TaskQueue* const task_queue_; - const rtc::WeakPtr ptr_; - }; - - RTC_DCHECK(config_.schedule_periodic_compound_packets); - - auto task = absl::make_unique( - config_.task_queue, ptr_factory_.GetWeakPtr()); if (delay_ms > 0) config_.task_queue->PostDelayedTask(std::move(task), delay_ms); else diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h index 7bcd16fa71..c97b2c5c11 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h @@ -24,8 +24,8 @@ #include "modules/rtp_rtcp/source/rtcp_packet/report_block.h" #include "modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h" #include "modules/rtp_rtcp/source/rtcp_transceiver_config.h" +#include "rtc_base/cancelable_task_handle.h" #include "rtc_base/constructormagic.h" -#include "rtc_base/weak_ptr.h" #include "system_wrappers/include/ntp_time.h" namespace webrtc { @@ -95,7 +95,7 @@ class RtcpTransceiverImpl { // TODO(danilchap): Remove entries from remote_senders_ that are no longer // needed. std::map remote_senders_; - rtc::WeakPtrFactory ptr_factory_; + rtc::CancelableTaskHandle periodic_task_handle_; RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RtcpTransceiverImpl); }; diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc b/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc index 1675b668e9..e5db0862cc 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc @@ -137,6 +137,40 @@ RtcpTransceiverConfig DefaultTestConfig() { return config; } +TEST(RtcpTransceiverImplTest, CanDestroyOnTaskQueue) { + FakeRtcpTransport transport; + rtc::TaskQueue queue("rtcp"); + RtcpTransceiverConfig config = DefaultTestConfig(); + config.task_queue = &queue; + config.schedule_periodic_compound_packets = true; + config.outgoing_transport = &transport; + auto* rtcp_transceiver = new RtcpTransceiverImpl(config); + // Wait for a periodic packet. + EXPECT_TRUE(transport.WaitPacket()); + + rtc::Event done(false, false); + queue.PostTask([rtcp_transceiver, &done] { + delete rtcp_transceiver; + done.Set(); + }); + ASSERT_TRUE(done.Wait(/*milliseconds=*/1000)); +} + +TEST(RtcpTransceiverImplTest, CanDestroyAfterTaskQueue) { + FakeRtcpTransport transport; + auto* queue = new rtc::TaskQueue("rtcp"); + RtcpTransceiverConfig config = DefaultTestConfig(); + config.task_queue = queue; + config.schedule_periodic_compound_packets = true; + config.outgoing_transport = &transport; + auto* rtcp_transceiver = new RtcpTransceiverImpl(config); + // Wait for a periodic packet. + EXPECT_TRUE(transport.WaitPacket()); + + delete queue; + delete rtcp_transceiver; +} + TEST(RtcpTransceiverImplTest, DelaysSendingFirstCompondPacket) { rtc::TaskQueue queue("rtcp"); FakeRtcpTransport transport; @@ -290,7 +324,7 @@ TEST(RtcpTransceiverImplTest, SendsPeriodicRtcpWhenNetworkStateIsUp) { absl::optional rtcp_transceiver; rtcp_transceiver.emplace(config); - rtcp_transceiver->SetReadyToSend(true); + queue.PostTask([&] { rtcp_transceiver->SetReadyToSend(true); }); EXPECT_TRUE(transport.WaitPacket());