From 7ac374abd7bafcc6f0a3ef4b4a8952a52c48d2f1 Mon Sep 17 00:00:00 2001 From: "mflodman@webrtc.org" Date: Fri, 20 Feb 2015 12:45:40 +0000 Subject: [PATCH] Fix shutdown race for ViEEncoder when there is a frame in the encoder. There is a potential race when deleting a channel and there is a frame in the encoder. ViEEncoder::SendData can be called after ViEEncoder::StopThreadsAndRemovePayloadRouter and payload_router is then already removed. Until we have the new API in place, use scoped_refptr in ViEChannel and ViEEncoder and deregister channel/encoder before deleting. BUG=769 R=stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/42019004 Cr-Commit-Position: refs/heads/master@{#8443} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8443 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/video_engine/payload_router.h | 6 ++++++ webrtc/video_engine/vie_channel.cc | 4 ++-- webrtc/video_engine/vie_channel.h | 5 +++-- webrtc/video_engine/vie_channel_manager.cc | 1 + webrtc/video_engine/vie_encoder.cc | 3 +-- webrtc/video_engine/vie_encoder.h | 6 ++++-- 6 files changed, 17 insertions(+), 8 deletions(-) diff --git a/webrtc/video_engine/payload_router.h b/webrtc/video_engine/payload_router.h index 60f4747223..ec9de993dc 100644 --- a/webrtc/video_engine/payload_router.h +++ b/webrtc/video_engine/payload_router.h @@ -17,6 +17,7 @@ #include "webrtc/base/constructormagic.h" #include "webrtc/base/thread_annotations.h" #include "webrtc/common_types.h" +#include "webrtc/system_wrappers/interface/atomic32.h" #include "webrtc/system_wrappers/interface/scoped_ptr.h" namespace webrtc { @@ -68,6 +69,9 @@ class PayloadRouter { // and RTP headers. size_t MaxPayloadLength() const; + void AddRef() { ++ref_count_; } + void Release() { if (--ref_count_ == 0) { delete this; } } + private: // TODO(mflodman): When the new video API has launched, remove crit_ and // assume rtp_modules_ will never change during a call. @@ -77,6 +81,8 @@ class PayloadRouter { std::vector rtp_modules_ GUARDED_BY(crit_.get()); bool active_ GUARDED_BY(crit_.get()); + Atomic32 ref_count_; + DISALLOW_COPY_AND_ASSIGN(PayloadRouter); }; diff --git a/webrtc/video_engine/vie_channel.cc b/webrtc/video_engine/vie_channel.cc index 588e1a94bf..95cba4084f 100644 --- a/webrtc/video_engine/vie_channel.cc +++ b/webrtc/video_engine/vie_channel.cc @@ -1509,8 +1509,8 @@ RtpRtcp* ViEChannel::rtp_rtcp() { return rtp_rtcp_.get(); } -PayloadRouter* ViEChannel::send_payload_router() { - return send_payload_router_.get(); +scoped_refptr ViEChannel::send_payload_router() { + return send_payload_router_; } CallStatsObserver* ViEChannel::GetStatsObserver() { diff --git a/webrtc/video_engine/vie_channel.h b/webrtc/video_engine/vie_channel.h index 093e179dda..59ec250d91 100644 --- a/webrtc/video_engine/vie_channel.h +++ b/webrtc/video_engine/vie_channel.h @@ -19,6 +19,7 @@ #include "webrtc/modules/video_coding/main/interface/video_coding_defines.h" #include "webrtc/system_wrappers/interface/critical_section_wrapper.h" #include "webrtc/system_wrappers/interface/scoped_ptr.h" +#include "webrtc/system_wrappers/interface/scoped_refptr.h" #include "webrtc/system_wrappers/interface/tick_util.h" #include "webrtc/typedefs.h" #include "webrtc/video_engine/include/vie_network.h" @@ -303,7 +304,7 @@ class ViEChannel // Gets the modules used by the channel. RtpRtcp* rtp_rtcp(); - PayloadRouter* send_payload_router(); + scoped_refptr send_payload_router(); CallStatsObserver* GetStatsObserver(); @@ -495,7 +496,7 @@ class ViEChannel scoped_ptr rtp_rtcp_; std::list simulcast_rtp_rtcp_; std::list removed_rtp_rtcp_; - scoped_ptr send_payload_router_; + scoped_refptr send_payload_router_; VideoCodingModule* const vcm_; ViEReceiver vie_receiver_; diff --git a/webrtc/video_engine/vie_channel_manager.cc b/webrtc/video_engine/vie_channel_manager.cc index c9f0f4ef0a..d0fcaaf75a 100644 --- a/webrtc/video_engine/vie_channel_manager.cc +++ b/webrtc/video_engine/vie_channel_manager.cc @@ -20,6 +20,7 @@ #include "webrtc/system_wrappers/interface/logging.h" #include "webrtc/video_engine/call_stats.h" #include "webrtc/video_engine/encoder_state_feedback.h" +#include "webrtc/video_engine/payload_router.h" #include "webrtc/video_engine/vie_channel.h" #include "webrtc/video_engine/vie_defines.h" #include "webrtc/video_engine/vie_encoder.h" diff --git a/webrtc/video_engine/vie_encoder.cc b/webrtc/video_engine/vie_encoder.cc index 9d4fdb2071..cecb3a2a96 100644 --- a/webrtc/video_engine/vie_encoder.cc +++ b/webrtc/video_engine/vie_encoder.cc @@ -229,7 +229,7 @@ bool ViEEncoder::Init() { } void ViEEncoder::StartThreadsAndSetSendPayloadRouter( - PayloadRouter* send_payload_router) { + scoped_refptr send_payload_router) { DCHECK(send_payload_router_ == NULL); send_payload_router_ = send_payload_router; @@ -245,7 +245,6 @@ void ViEEncoder::StopThreadsAndRemovePayloadRouter() { module_process_thread_.DeRegisterModule(&vcm_); module_process_thread_.DeRegisterModule(&vpm_); module_process_thread_.DeRegisterModule(default_rtp_rtcp_.get()); - send_payload_router_ = nullptr; } ViEEncoder::~ViEEncoder() { diff --git a/webrtc/video_engine/vie_encoder.h b/webrtc/video_engine/vie_encoder.h index d9c6847f0a..34120bcb96 100644 --- a/webrtc/video_engine/vie_encoder.h +++ b/webrtc/video_engine/vie_encoder.h @@ -24,6 +24,7 @@ #include "webrtc/modules/video_processing/main/interface/video_processing.h" #include "webrtc/typedefs.h" #include "webrtc/frame_callback.h" +#include "webrtc/system_wrappers/interface/scoped_refptr.h" #include "webrtc/video_engine/vie_defines.h" #include "webrtc/video_engine/vie_frame_provider_base.h" @@ -69,7 +70,8 @@ class ViEEncoder // only once. // Ideally this would be done in Init, but the dependencies between ViEEncoder // and ViEChannel makes it really hard to do in a good way. - void StartThreadsAndSetSendPayloadRouter(PayloadRouter* send_payload_router); + void StartThreadsAndSetSendPayloadRouter( + scoped_refptr send_payload_router); // This function must be called before the corresponding ViEChannel is // deleted. @@ -215,7 +217,7 @@ class ViEEncoder VideoCodingModule& vcm_; VideoProcessingModule& vpm_; rtc::scoped_ptr default_rtp_rtcp_; - PayloadRouter* send_payload_router_; + scoped_refptr send_payload_router_; rtc::scoped_ptr callback_cs_; rtc::scoped_ptr data_cs_; rtc::scoped_ptr bitrate_observer_;