From 4ec56a3aa0b402fe37252f326e21cd2909874aa8 Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Fri, 14 Apr 2023 17:03:47 +0200 Subject: [PATCH] VCMJitterBuffer: fix deadlock. The jitterbuffer would call Flush which takes a mutex from InsertPacket, which is already under the same mutex. Fix this by introducing an internal flush method that assumes a locked state. The change also adds more thread annotations in case more problems were present. No more problems were detected. Fixed: b/277930190 Change-Id: If85609f27f8187ade9370847fecc2bc83d940dd5 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301340 Commit-Queue: Ilya Nikolaevskiy Auto-Submit: Markus Handell Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/main@{#39868} --- modules/video_coding/jitter_buffer.cc | 6 ++++- modules/video_coding/jitter_buffer.h | 38 ++++++++++++++++----------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/modules/video_coding/jitter_buffer.cc b/modules/video_coding/jitter_buffer.cc index e417ab3048..24e4fd2d8b 100644 --- a/modules/video_coding/jitter_buffer.cc +++ b/modules/video_coding/jitter_buffer.cc @@ -186,6 +186,10 @@ bool VCMJitterBuffer::Running() const { void VCMJitterBuffer::Flush() { MutexLock lock(&mutex_); + FlushInternal(); +} + +void VCMJitterBuffer::FlushInternal() { decodable_frames_.Reset(&free_frames_); incomplete_frames_.Reset(&free_frames_); last_decoded_state_.Reset(); // TODO(mikhal): sync reset. @@ -378,7 +382,7 @@ VCMFrameBufferEnum VCMJitterBuffer::InsertPacket(const VCMPacket& packet, RTC_LOG(LS_WARNING) << num_consecutive_old_packets_ << " consecutive old packets received. Flushing the jitter buffer."; - Flush(); + FlushInternal(); return kFlushIndicator; } return kOldPacket; diff --git a/modules/video_coding/jitter_buffer.h b/modules/video_coding/jitter_buffer.h index 6563c56e87..fe314f0104 100644 --- a/modules/video_coding/jitter_buffer.h +++ b/modules/video_coding/jitter_buffer.h @@ -80,55 +80,59 @@ class VCMJitterBuffer { VCMJitterBuffer& operator=(const VCMJitterBuffer&) = delete; // Initializes and starts jitter buffer. - void Start(); + void Start() RTC_LOCKS_EXCLUDED(mutex_); // Signals all internal events and stops the jitter buffer. - void Stop(); + void Stop() RTC_LOCKS_EXCLUDED(mutex_); // Returns true if the jitter buffer is running. - bool Running() const; + bool Running() const RTC_LOCKS_EXCLUDED(mutex_); // Empty the jitter buffer of all its data. - void Flush(); + void Flush() RTC_LOCKS_EXCLUDED(mutex_); // Gets number of packets received. - int num_packets() const; + int num_packets() const RTC_LOCKS_EXCLUDED(mutex_); // Gets number of duplicated packets received. - int num_duplicated_packets() const; + int num_duplicated_packets() const RTC_LOCKS_EXCLUDED(mutex_); // Wait `max_wait_time_ms` for a complete frame to arrive. // If found, a pointer to the frame is returned. Returns nullptr otherwise. - VCMEncodedFrame* NextCompleteFrame(uint32_t max_wait_time_ms); + VCMEncodedFrame* NextCompleteFrame(uint32_t max_wait_time_ms) + RTC_LOCKS_EXCLUDED(mutex_); // Extract frame corresponding to input timestamp. // Frame will be set to a decoding state. - VCMEncodedFrame* ExtractAndSetDecode(uint32_t timestamp); + VCMEncodedFrame* ExtractAndSetDecode(uint32_t timestamp) + RTC_LOCKS_EXCLUDED(mutex_); // Releases a frame returned from the jitter buffer, should be called when // done with decoding. - void ReleaseFrame(VCMEncodedFrame* frame); + void ReleaseFrame(VCMEncodedFrame* frame) RTC_LOCKS_EXCLUDED(mutex_); // Returns the time in ms when the latest packet was inserted into the frame. // Retransmitted is set to true if any of the packets belonging to the frame // has been retransmitted. int64_t LastPacketTime(const VCMEncodedFrame* frame, - bool* retransmitted) const; + bool* retransmitted) const RTC_LOCKS_EXCLUDED(mutex_); // Inserts a packet into a frame returned from GetFrame(). // If the return value is <= 0, `frame` is invalidated and the pointer must // be dropped after this function returns. - VCMFrameBufferEnum InsertPacket(const VCMPacket& packet, bool* retransmitted); + VCMFrameBufferEnum InsertPacket(const VCMPacket& packet, bool* retransmitted) + RTC_LOCKS_EXCLUDED(mutex_); // Returns the estimated jitter in milliseconds. - uint32_t EstimatedJitterMs(); + uint32_t EstimatedJitterMs() RTC_LOCKS_EXCLUDED(mutex_); void SetNackSettings(size_t max_nack_list_size, int max_packet_age_to_nack, - int max_incomplete_time_ms); + int max_incomplete_time_ms) RTC_LOCKS_EXCLUDED(mutex_); // Returns a list of the sequence numbers currently missing. - std::vector GetNackList(bool* request_key_frame); + std::vector GetNackList(bool* request_key_frame) + RTC_LOCKS_EXCLUDED(mutex_); private: class SequenceNumberLessThan { @@ -202,7 +206,8 @@ class VCMJitterBuffer { bool RecycleFramesUntilKeyFrame() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); // Update rolling average of packets per frame. - void UpdateAveragePacketsPerFrame(int current_number_packets_); + void UpdateAveragePacketsPerFrame(int current_number_packets_) + RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); // Cleans the frame list in the JB from old/empty frames. // Should only be called prior to actual use. @@ -229,6 +234,9 @@ class VCMJitterBuffer { void RecycleFrameBuffer(VCMFrameBuffer* frame) RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); + // Empty the jitter buffer of all its data. + void FlushInternal() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); + Clock* clock_; // If we are running (have started) or not. bool running_;