Remove PacketBuffers internal mutex.
In RtpVideoStreamReceiver2 it can be protected by the `worker_task_checker_` instead.
Bug: webrtc:12579
Change-Id: I4f7d64f16172139eddc7a3e07d1dbbf338beaf2e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215224
Commit-Queue: Philip Eliasson <philipel@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33734}
diff --git a/modules/video_coding/packet_buffer.cc b/modules/video_coding/packet_buffer.cc
index 0d4c085..1291f78 100644
--- a/modules/video_coding/packet_buffer.cc
+++ b/modules/video_coding/packet_buffer.cc
@@ -70,7 +70,6 @@
PacketBuffer::InsertResult PacketBuffer::InsertPacket(
std::unique_ptr<PacketBuffer::Packet> packet) {
PacketBuffer::InsertResult result;
- MutexLock lock(&mutex_);
uint16_t seq_num = packet->seq_num;
size_t index = seq_num % buffer_.size();
@@ -120,7 +119,6 @@
}
void PacketBuffer::ClearTo(uint16_t seq_num) {
- MutexLock lock(&mutex_);
// We have already cleared past this sequence number, no need to do anything.
if (is_cleared_to_first_seq_num_ &&
AheadOf<uint16_t>(first_seq_num_, seq_num)) {
@@ -157,13 +155,11 @@
}
void PacketBuffer::Clear() {
- MutexLock lock(&mutex_);
ClearInternal();
}
PacketBuffer::InsertResult PacketBuffer::InsertPadding(uint16_t seq_num) {
PacketBuffer::InsertResult result;
- MutexLock lock(&mutex_);
UpdateMissingPackets(seq_num);
result.packets = FindFrames(static_cast<uint16_t>(seq_num + 1));
return result;
diff --git a/modules/video_coding/packet_buffer.h b/modules/video_coding/packet_buffer.h
index eb8d836..c0cc752 100644
--- a/modules/video_coding/packet_buffer.h
+++ b/modules/video_coding/packet_buffer.h
@@ -23,7 +23,6 @@
#include "modules/rtp_rtcp/source/rtp_video_header.h"
#include "rtc_base/copy_on_write_buffer.h"
#include "rtc_base/numerics/sequence_number_util.h"
-#include "rtc_base/synchronization/mutex.h"
#include "rtc_base/thread_annotations.h"
namespace webrtc {
@@ -78,55 +77,47 @@
PacketBuffer(size_t start_buffer_size, size_t max_buffer_size);
~PacketBuffer();
- ABSL_MUST_USE_RESULT InsertResult InsertPacket(std::unique_ptr<Packet> packet)
- RTC_LOCKS_EXCLUDED(mutex_);
- ABSL_MUST_USE_RESULT InsertResult InsertPadding(uint16_t seq_num)
- RTC_LOCKS_EXCLUDED(mutex_);
- void ClearTo(uint16_t seq_num) RTC_LOCKS_EXCLUDED(mutex_);
- void Clear() RTC_LOCKS_EXCLUDED(mutex_);
+ ABSL_MUST_USE_RESULT InsertResult
+ InsertPacket(std::unique_ptr<Packet> packet);
+ ABSL_MUST_USE_RESULT InsertResult InsertPadding(uint16_t seq_num);
+ void ClearTo(uint16_t seq_num);
+ void Clear();
void ForceSpsPpsIdrIsH264Keyframe();
private:
- // Clears with |mutex_| taken.
- void ClearInternal() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+ void ClearInternal();
// Tries to expand the buffer.
- bool ExpandBufferSize() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+ bool ExpandBufferSize();
// Test if all previous packets has arrived for the given sequence number.
- bool PotentialNewFrame(uint16_t seq_num) const
- RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+ bool PotentialNewFrame(uint16_t seq_num) const;
// Test if all packets of a frame has arrived, and if so, returns packets to
// create frames.
- std::vector<std::unique_ptr<Packet>> FindFrames(uint16_t seq_num)
- RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+ std::vector<std::unique_ptr<Packet>> FindFrames(uint16_t seq_num);
- void UpdateMissingPackets(uint16_t seq_num)
- RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
-
- mutable Mutex mutex_;
+ void UpdateMissingPackets(uint16_t seq_num);
// buffer_.size() and max_size_ must always be a power of two.
const size_t max_size_;
// The fist sequence number currently in the buffer.
- uint16_t first_seq_num_ RTC_GUARDED_BY(mutex_);
+ uint16_t first_seq_num_;
// If the packet buffer has received its first packet.
- bool first_packet_received_ RTC_GUARDED_BY(mutex_);
+ bool first_packet_received_;
// If the buffer is cleared to |first_seq_num_|.
- bool is_cleared_to_first_seq_num_ RTC_GUARDED_BY(mutex_);
+ bool is_cleared_to_first_seq_num_;
// Buffer that holds the the inserted packets and information needed to
// determine continuity between them.
- std::vector<std::unique_ptr<Packet>> buffer_ RTC_GUARDED_BY(mutex_);
+ std::vector<std::unique_ptr<Packet>> buffer_;
- absl::optional<uint16_t> newest_inserted_seq_num_ RTC_GUARDED_BY(mutex_);
- std::set<uint16_t, DescendingSeqNumComp<uint16_t>> missing_packets_
- RTC_GUARDED_BY(mutex_);
+ absl::optional<uint16_t> newest_inserted_seq_num_;
+ std::set<uint16_t, DescendingSeqNumComp<uint16_t>> missing_packets_;
// Indicates if we should require SPS, PPS, and IDR for a particular
// RTP timestamp to treat the corresponding frame as a keyframe.
diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc
index e092fe7..a5937b0 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -364,6 +364,7 @@
bool raw_payload) {
if (codec_params.count(cricket::kH264FmtpSpsPpsIdrInKeyframe) ||
field_trial::IsEnabled("WebRTC-SpsPpsIdrIsH264Keyframe")) {
+ MutexLock lock(&packet_buffer_lock_);
packet_buffer_.ForceSpsPpsIdrIsH264Keyframe();
}
payload_type_map_.emplace(
@@ -644,7 +645,12 @@
rtcp_feedback_buffer_.SendBufferedRtcpFeedback();
frame_counter_.Add(packet->timestamp);
- OnInsertedPacket(packet_buffer_.InsertPacket(std::move(packet)));
+ video_coding::PacketBuffer::InsertResult insert_result;
+ {
+ MutexLock lock(&packet_buffer_lock_);
+ insert_result = packet_buffer_.InsertPacket(std::move(packet));
+ }
+ OnInsertedPacket(std::move(insert_result));
}
void RtpVideoStreamReceiver::OnRecoveredPacket(const uint8_t* rtp_packet,
@@ -1016,7 +1022,12 @@
MutexLock lock(&reference_finder_lock_);
reference_finder_->PaddingReceived(seq_num);
}
- OnInsertedPacket(packet_buffer_.InsertPadding(seq_num));
+ video_coding::PacketBuffer::InsertResult insert_result;
+ {
+ MutexLock lock(&packet_buffer_lock_);
+ insert_result = packet_buffer_.InsertPadding(seq_num);
+ }
+ OnInsertedPacket(std::move(insert_result));
if (nack_module_) {
nack_module_->OnReceivedPacket(seq_num, /* is_keyframe = */ false,
/* is _recovered = */ false);
@@ -1098,7 +1109,10 @@
}
}
if (seq_num != -1) {
- packet_buffer_.ClearTo(seq_num);
+ {
+ MutexLock lock(&packet_buffer_lock_);
+ packet_buffer_.ClearTo(seq_num);
+ }
MutexLock lock(&reference_finder_lock_);
reference_finder_->ClearTo(seq_num);
}
diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h
index 7a84519..b275fb6 100644
--- a/video/rtp_video_stream_receiver.h
+++ b/video/rtp_video_stream_receiver.h
@@ -337,7 +337,8 @@
std::unique_ptr<DEPRECATED_NackModule> nack_module_;
std::unique_ptr<LossNotificationController> loss_notification_controller_;
- video_coding::PacketBuffer packet_buffer_;
+ mutable Mutex packet_buffer_lock_;
+ video_coding::PacketBuffer packet_buffer_ RTC_GUARDED_BY(packet_buffer_lock_);
UniqueTimestampCounter frame_counter_ RTC_GUARDED_BY(worker_task_checker_);
SeqNumUnwrapper<uint16_t> frame_id_unwrapper_
RTC_GUARDED_BY(worker_task_checker_);
diff --git a/video/rtp_video_stream_receiver2.h b/video/rtp_video_stream_receiver2.h
index 207dfe7..4076837 100644
--- a/video/rtp_video_stream_receiver2.h
+++ b/video/rtp_video_stream_receiver2.h
@@ -294,7 +294,8 @@
const std::unique_ptr<NackModule2> nack_module_;
std::unique_ptr<LossNotificationController> loss_notification_controller_;
- video_coding::PacketBuffer packet_buffer_;
+ video_coding::PacketBuffer packet_buffer_
+ RTC_GUARDED_BY(worker_task_checker_);
UniqueTimestampCounter frame_counter_ RTC_GUARDED_BY(worker_task_checker_);
SeqNumUnwrapper<uint16_t> frame_id_unwrapper_
RTC_GUARDED_BY(worker_task_checker_);
