From 22691e0d3273649a7bc5e6822e4c82f3b02ad5ae Mon Sep 17 00:00:00 2001 From: sprang Date: Wed, 13 Jul 2016 10:57:07 -0700 Subject: [PATCH] Avoid race in VideoReceiveStream shutdown The decode thread should be stopped before triggering shutdown of the video receiver, so that the decoder doesn't try to insert a new frame while the jitter buffer is being shut down. BUG=webrtc:6102 Review-Url: https://codereview.webrtc.org/2146883002 Cr-Commit-Position: refs/heads/master@{#13467} --- webrtc/modules/video_coding/jitter_buffer.cc | 46 ++++++++------------ webrtc/modules/video_coding/jitter_buffer.h | 8 ++-- webrtc/video/video_receive_stream.cc | 3 ++ 3 files changed, 26 insertions(+), 31 deletions(-) diff --git a/webrtc/modules/video_coding/jitter_buffer.cc b/webrtc/modules/video_coding/jitter_buffer.cc index 7cd874cabf..02d8afa496 100644 --- a/webrtc/modules/video_coding/jitter_buffer.cc +++ b/webrtc/modules/video_coding/jitter_buffer.cc @@ -229,7 +229,7 @@ VCMJitterBuffer::VCMJitterBuffer(Clock* clock, incomplete_frames_(), last_decoded_state_(), first_packet_since_reset_(true), - stats_callback_(NULL), + stats_callback_(nullptr), incoming_frame_rate_(0), incoming_frame_count_(0), time_last_incoming_frame_count_(0), @@ -247,6 +247,7 @@ VCMJitterBuffer::VCMJitterBuffer(Clock* clock, low_rtt_nack_threshold_ms_(-1), high_rtt_nack_threshold_ms_(-1), missing_sequence_numbers_(SequenceNumberLessThan()), + latest_received_sequence_number_(0), max_nack_list_size_(0), max_packet_age_to_nack_(0), max_incomplete_time_ms_(0), @@ -325,30 +326,17 @@ void VCMJitterBuffer::Start() { first_packet_since_reset_ = true; rtt_ms_ = kDefaultRtt; last_decoded_state_.Reset(); + + decodable_frames_.Reset(&free_frames_); + incomplete_frames_.Reset(&free_frames_); } void VCMJitterBuffer::Stop() { - crit_sect_->Enter(); + CriticalSectionScoped cs(crit_sect_); UpdateHistograms(); running_ = false; last_decoded_state_.Reset(); - // Make sure all frames are free and reset. - for (FrameList::iterator it = decodable_frames_.begin(); - it != decodable_frames_.end(); ++it) { - free_frames_.push_back(it->second); - } - for (FrameList::iterator it = incomplete_frames_.begin(); - it != incomplete_frames_.end(); ++it) { - free_frames_.push_back(it->second); - } - for (UnorderedFrameList::iterator it = free_frames_.begin(); - it != free_frames_.end(); ++it) { - (*it)->Reset(); - } - decodable_frames_.clear(); - incomplete_frames_.clear(); - crit_sect_->Leave(); // Make sure we wake up any threads waiting on these events. frame_event_->Set(); } @@ -594,11 +582,10 @@ VCMEncodedFrame* VCMJitterBuffer::ExtractAndSetDecode(uint32_t timestamp) { // Release frame when done with decoding. Should never be used to release // frames from within the jitter buffer. void VCMJitterBuffer::ReleaseFrame(VCMEncodedFrame* frame) { + RTC_CHECK(frame != nullptr); CriticalSectionScoped cs(crit_sect_); VCMFrameBuffer* frame_buffer = static_cast(frame); - if (frame_buffer) { - free_frames_.push_back(frame_buffer); - } + RecycleFrameBuffer(frame_buffer); } // Gets frame to use for this timestamp. If no match, get empty frame. @@ -624,9 +611,9 @@ VCMFrameBufferEnum VCMJitterBuffer::GetFrame(const VCMPacket& packet, LOG(LS_WARNING) << "Unable to get empty frame; Recycling."; bool found_key_frame = RecycleFramesUntilKeyFrame(); *frame = GetEmptyFrame(); - assert(*frame); + RTC_CHECK(*frame); if (!found_key_frame) { - free_frames_.push_back(*frame); + RecycleFrameBuffer(*frame); return kFlushIndicator; } } @@ -753,7 +740,7 @@ VCMFrameBufferEnum VCMJitterBuffer::InsertPacket(const VCMPacket& packet, case kGeneralError: case kTimeStampError: case kSizeError: { - free_frames_.push_back(frame); + RecycleFrameBuffer(frame); break; } case kCompleteSession: { @@ -787,7 +774,7 @@ VCMFrameBufferEnum VCMJitterBuffer::InsertPacket(const VCMPacket& packet, case kIncomplete: { if (frame->GetState() == kStateEmpty && last_decoded_state_.UpdateEmptyFrame(frame)) { - free_frames_.push_back(frame); + RecycleFrameBuffer(frame); return kNoError; } else { incomplete_frames_.InsertFrame(frame); @@ -807,13 +794,13 @@ VCMFrameBufferEnum VCMJitterBuffer::InsertPacket(const VCMPacket& packet, if (frame_list != NULL) { frame_list->InsertFrame(frame); } else { - free_frames_.push_back(frame); + RecycleFrameBuffer(frame); } ++num_duplicated_packets_; break; } case kFlushIndicator: - free_frames_.push_back(frame); + RecycleFrameBuffer(frame); return kFlushIndicator; default: assert(false); @@ -1315,4 +1302,9 @@ bool VCMJitterBuffer::WaitForRetransmissions() { return true; } +void VCMJitterBuffer::RecycleFrameBuffer(VCMFrameBuffer* frame) { + frame->Reset(); + free_frames_.push_back(frame); +} + } // namespace webrtc diff --git a/webrtc/modules/video_coding/jitter_buffer.h b/webrtc/modules/video_coding/jitter_buffer.h index 35b32c960d..62017dae7f 100644 --- a/webrtc/modules/video_coding/jitter_buffer.h +++ b/webrtc/modules/video_coding/jitter_buffer.h @@ -261,8 +261,6 @@ class VCMJitterBuffer { // Drops all packets in the NACK list up until |last_decoded_sequence_number|. void DropPacketsFromNackList(uint16_t last_decoded_sequence_number); - void ReleaseFrameIfNotDecoding(VCMFrameBuffer* frame); - // Gets an empty frame, creating a new frame if necessary (i.e. increases // jitter buffer size). VCMFrameBuffer* GetEmptyFrame() EXCLUSIVE_LOCKS_REQUIRED(crit_sect_); @@ -310,6 +308,10 @@ class VCMJitterBuffer { void UpdateHistograms() EXCLUSIVE_LOCKS_REQUIRED(crit_sect_); + // Reset frame buffer and return it to free_frames_. + void RecycleFrameBuffer(VCMFrameBuffer* frame) + EXCLUSIVE_LOCKS_REQUIRED(crit_sect_); + Clock* clock_; // If we are running (have started) or not. bool running_; @@ -334,8 +336,6 @@ class VCMJitterBuffer { int64_t time_last_incoming_frame_count_; unsigned int incoming_bit_count_; unsigned int incoming_bit_rate_; - // Number of frames in a row that have been too old. - int num_consecutive_old_frames_; // Number of packets in a row that have been too old. int num_consecutive_old_packets_; // Number of packets received. diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index ad9beb35e7..609a789fa2 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -267,6 +267,9 @@ void VideoReceiveStream::Start() { void VideoReceiveStream::Stop() { rtp_stream_receiver_.StopReceive(); + // TriggerDecoderShutdown will release any waiting decoder thread and make it + // stop immediately, instead of waiting for a timeout. Needs to be called + // before joining the decoder thread thread. video_receiver_.TriggerDecoderShutdown(); decode_thread_.Stop(); call_stats_->DeregisterStatsObserver(video_stream_decoder_.get());