From 98f5f6cdea65caa6557489a5604cf6713dcaa66b Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 22 Oct 2018 14:22:31 +0200 Subject: [PATCH] In RtcpTransceiver functions with callback avoid relying on PostTaskAndReply deprecated version guarantees (using PostTaskAndReply) callback task will run on the task queue, and thus doesn't guarantee to run it if task queue is destroyed, new callback versions instead guarantee callback will always run, but may run off the task queue if task queue is destroyed. Both keep guarantee observer callbacks will not run after on_destroyed/on_removed is called. Bug: None Change-Id: I61bf52127f3084c0186aa8bc89037bf9296801d8 Reviewed-on: https://webrtc-review.googlesource.com/c/107305 Commit-Queue: Danil Chapovalov Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#25301} --- modules/rtp_rtcp/BUILD.gn | 1 + modules/rtp_rtcp/source/rtcp_transceiver.cc | 20 ++++++++++++++ modules/rtp_rtcp/source/rtcp_transceiver.h | 10 ++++++- .../source/rtcp_transceiver_unittest.cc | 26 +++++++++---------- 4 files changed, 42 insertions(+), 15 deletions(-) diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn index 9102562974..fad15b1428 100644 --- a/modules/rtp_rtcp/BUILD.gn +++ b/modules/rtp_rtcp/BUILD.gn @@ -243,6 +243,7 @@ rtc_source_set("rtcp_transceiver") { "../../api:transport_api", "../../api/video:video_bitrate_allocation", "../../rtc_base:checks", + "../../rtc_base:deprecation", "../../rtc_base:rtc_base_approved", "../../rtc_base:rtc_cancelable_task", "../../rtc_base:rtc_task_queue", diff --git a/modules/rtp_rtcp/source/rtcp_transceiver.cc b/modules/rtp_rtcp/source/rtcp_transceiver.cc index 77b10ca052..9c48b993ac 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver.cc @@ -46,6 +46,13 @@ void RtcpTransceiver::Stop(std::unique_ptr on_destroyed) { RTC_DCHECK(!rtcp_transceiver_); } +void RtcpTransceiver::Stop(std::function on_destroyed) { + RTC_DCHECK(rtcp_transceiver_); + task_queue_->PostTask(rtc::NewClosure( + Destructor{std::move(rtcp_transceiver_)}, std::move(on_destroyed))); + RTC_DCHECK(!rtcp_transceiver_); +} + void RtcpTransceiver::AddMediaReceiverRtcpObserver( uint32_t remote_ssrc, MediaReceiverRtcpObserver* observer) { @@ -68,6 +75,19 @@ void RtcpTransceiver::RemoveMediaReceiverRtcpObserver( task_queue_->PostTaskAndReply(std::move(remove), std::move(on_removed)); } +void RtcpTransceiver::RemoveMediaReceiverRtcpObserver( + uint32_t remote_ssrc, + MediaReceiverRtcpObserver* observer, + std::function on_removed) { + RTC_CHECK(rtcp_transceiver_); + RtcpTransceiverImpl* ptr = rtcp_transceiver_.get(); + auto remove = [ptr, remote_ssrc, observer] { + ptr->RemoveMediaReceiverRtcpObserver(remote_ssrc, observer); + }; + task_queue_->PostTask( + rtc::NewClosure(std::move(remove), std::move(on_removed))); +} + void RtcpTransceiver::SetReadyToSend(bool ready) { RTC_CHECK(rtcp_transceiver_); RtcpTransceiverImpl* ptr = rtcp_transceiver_.get(); diff --git a/modules/rtp_rtcp/source/rtcp_transceiver.h b/modules/rtp_rtcp/source/rtcp_transceiver.h index fc9488c4ab..e18496fbff 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver.h +++ b/modules/rtp_rtcp/source/rtcp_transceiver.h @@ -11,6 +11,7 @@ #ifndef MODULES_RTP_RTCP_SOURCE_RTCP_TRANSCEIVER_H_ #define MODULES_RTP_RTCP_SOURCE_RTCP_TRANSCEIVER_H_ +#include #include #include #include @@ -18,6 +19,7 @@ #include "modules/rtp_rtcp/source/rtcp_transceiver_config.h" #include "modules/rtp_rtcp/source/rtcp_transceiver_impl.h" #include "rtc_base/copyonwritebuffer.h" +#include "rtc_base/deprecation.h" #include "rtc_base/task_queue.h" namespace webrtc { @@ -42,18 +44,24 @@ class RtcpTransceiver : public RtcpFeedbackSenderInterface { // Note that interfaces provided in constructor or registered with AddObserver // still might be used by the transceiver on the task queue // until |on_destroyed| runs. + RTC_DEPRECATED void Stop(std::unique_ptr on_destroyed); + void Stop(std::function on_destroyed); // Registers observer to be notified about incoming rtcp packets. // Calls to observer will be done on the |config.task_queue|. void AddMediaReceiverRtcpObserver(uint32_t remote_ssrc, MediaReceiverRtcpObserver* observer); // Deregisters the observer. Might return before observer is deregistered. - // Posts |on_removed| task when observer is deregistered. + // Runs |on_removed| when observer is deregistered. + RTC_DEPRECATED void RemoveMediaReceiverRtcpObserver( uint32_t remote_ssrc, MediaReceiverRtcpObserver* observer, std::unique_ptr on_removed); + void RemoveMediaReceiverRtcpObserver(uint32_t remote_ssrc, + MediaReceiverRtcpObserver* observer, + std::function on_removed); // Enables/disables sending rtcp packets eventually. // Packets may be sent after the SetReadyToSend(false) returns, but no new diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc index 2ea5bc9b53..aaaf9222dc 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver_unittest.cc @@ -166,12 +166,11 @@ TEST(RtcpTransceiverTest, DoesntPostToRtcpObserverAfterCallToRemove) { rtcp_transceiver.AddMediaReceiverRtcpObserver(kRemoteSsrc, observer.get()); rtcp_transceiver.ReceivePacket(CreateSenderReport(kRemoteSsrc, 1)); - rtcp_transceiver.RemoveMediaReceiverRtcpObserver( - kRemoteSsrc, observer.get(), - /*on_removed=*/rtc::NewClosure([&] { - observer.reset(); - observer_deleted.Set(); - })); + rtcp_transceiver.RemoveMediaReceiverRtcpObserver(kRemoteSsrc, observer.get(), + /*on_removed=*/[&] { + observer.reset(); + observer_deleted.Set(); + }); rtcp_transceiver.ReceivePacket(CreateSenderReport(kRemoteSsrc, 2)); EXPECT_TRUE(observer_deleted.Wait(kTimeoutMs)); @@ -192,12 +191,11 @@ TEST(RtcpTransceiverTest, RemoveMediaReceiverRtcpObserverIsNonBlocking) { rtc::Event queue_blocker(false, false); rtc::Event observer_deleted(false, false); queue.PostTask([&] { EXPECT_TRUE(queue_blocker.Wait(kTimeoutMs)); }); - rtcp_transceiver.RemoveMediaReceiverRtcpObserver( - kRemoteSsrc, observer.get(), - /*on_removed=*/rtc::NewClosure([&] { - observer.reset(); - observer_deleted.Set(); - })); + rtcp_transceiver.RemoveMediaReceiverRtcpObserver(kRemoteSsrc, observer.get(), + /*on_removed=*/[&] { + observer.reset(); + observer_deleted.Set(); + }); EXPECT_THAT(observer, Not(IsNull())); queue_blocker.Set(); @@ -244,10 +242,10 @@ TEST(RtcpTransceiverTest, DoesntSendPacketsAfterStopCallback) { auto rtcp_transceiver = absl::make_unique(config); rtc::Event done(false, false); rtcp_transceiver->SendCompoundPacket(); - rtcp_transceiver->Stop(rtc::NewClosure([&] { + rtcp_transceiver->Stop([&] { EXPECT_CALL(outgoing_transport, SendRtcp).Times(0); done.Set(); - })); + }); rtcp_transceiver = nullptr; EXPECT_TRUE(done.Wait(kTimeoutMs)); }