From 52b4e8871a7c43a12177cb9a717baff3fb2680c0 Mon Sep 17 00:00:00 2001 From: "pwestin@webrtc.org" Date: Thu, 2 May 2013 19:02:17 +0000 Subject: [PATCH] Adding trace and changing pacing constants BUG=1721,1722 R=mikhal@webrtc.org, niklas.enbom@webrtc.org Review URL: https://webrtc-codereview.appspot.com/1380005 git-svn-id: http://webrtc.googlecode.com/svn/trunk@3940 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../pacing/include/mock/mock_paced_sender.h | 2 +- webrtc/modules/pacing/include/paced_sender.h | 12 +++- webrtc/modules/pacing/paced_sender.cc | 66 ++++++++++++++----- .../modules/pacing/paced_sender_unittest.cc | 4 +- webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 10 +-- webrtc/video_engine/vie_encoder.cc | 32 ++++++++- webrtc/video_engine/vie_encoder.h | 1 + 7 files changed, 97 insertions(+), 30 deletions(-) diff --git a/webrtc/modules/pacing/include/mock/mock_paced_sender.h b/webrtc/modules/pacing/include/mock/mock_paced_sender.h index 13b414dea..b562c38fb 100644 --- a/webrtc/modules/pacing/include/mock/mock_paced_sender.h +++ b/webrtc/modules/pacing/include/mock/mock_paced_sender.h @@ -21,7 +21,7 @@ namespace webrtc { class MockPacedSender : public PacedSender { public: - MockPacedSender() : PacedSender(NULL, 0) {} + MockPacedSender() : PacedSender(NULL, 0, 0) {} MOCK_METHOD5(SendPacket, bool(Priority priority, uint32_t ssrc, uint16_t sequence_number, diff --git a/webrtc/modules/pacing/include/paced_sender.h b/webrtc/modules/pacing/include/paced_sender.h index bd4880de4..837af27b2 100644 --- a/webrtc/modules/pacing/include/paced_sender.h +++ b/webrtc/modules/pacing/include/paced_sender.h @@ -44,7 +44,8 @@ class PacedSender : public Module { protected: virtual ~Callback() {} }; - PacedSender(Callback* callback, int target_bitrate_kbps); + PacedSender(Callback* callback, int target_bitrate_kbps, + float pace_multiplier); virtual ~PacedSender(); @@ -110,11 +111,13 @@ class PacedSender : public Module { // Checks if next packet in line can be transmitted. Returns true on success. bool GetNextPacket(uint32_t* ssrc, uint16_t* sequence_number, - int64_t* capture_time_ms); + int64_t* capture_time_ms, Priority* priority, + bool* last_packet); // Local helper function to GetNextPacket. void GetNextPacketFromList(PacketList* list, - uint32_t* ssrc, uint16_t* sequence_number, int64_t* capture_time_ms); + uint32_t* ssrc, uint16_t* sequence_number, int64_t* capture_time_ms, + bool* last_packet); // Updates the number of bytes that can be sent for the next time interval. void UpdateBytesPerInterval(uint32_t delta_time_in_ms); @@ -123,6 +126,7 @@ class PacedSender : public Module { void UpdateState(int num_bytes); Callback* callback_; + const float pace_multiplier_; bool enable_; bool paused_; scoped_ptr critsect_; @@ -131,6 +135,8 @@ class PacedSender : public Module { int padding_bytes_remaining_interval_; TickTime time_last_update_; TickTime time_last_send_; + int64_t capture_time_ms_last_queued_; + int64_t capture_time_ms_last_sent_; PacketList high_priority_packets_; PacketList normal_priority_packets_; diff --git a/webrtc/modules/pacing/paced_sender.cc b/webrtc/modules/pacing/paced_sender.cc index 1d2c7006e..6c9939bfb 100644 --- a/webrtc/modules/pacing/paced_sender.cc +++ b/webrtc/modules/pacing/paced_sender.cc @@ -12,15 +12,11 @@ #include +#include "webrtc/modules/interface/module_common_types.h" #include "webrtc/system_wrappers/interface/critical_section_wrapper.h" +#include "webrtc/system_wrappers/interface/trace_event.h" namespace { -// Multiplicative factor that is applied to the target bitrate to calculate the -// number of bytes that can be transmitted per interval. -// Increasing this factor will result in lower delays in cases of bitrate -// overshoots from the encoder. -const float kBytesPerIntervalMargin = 1.5f; - // Time limit in milliseconds between packet bursts. const int kMinPacketLimitMs = 5; @@ -59,15 +55,19 @@ void PacedSender::PacketList::push_back(const PacedSender::Packet& packet) { } } -PacedSender::PacedSender(Callback* callback, int target_bitrate_kbps) +PacedSender::PacedSender(Callback* callback, int target_bitrate_kbps, + float pace_multiplier) : callback_(callback), + pace_multiplier_(pace_multiplier), enable_(false), paused_(false), critsect_(CriticalSectionWrapper::CreateCriticalSection()), target_bitrate_kbytes_per_s_(target_bitrate_kbps >> 3), // Divide by 8. bytes_remaining_interval_(0), padding_bytes_remaining_interval_(0), - time_last_update_(TickTime::Now()) { + time_last_update_(TickTime::Now()), + capture_time_ms_last_queued_(0), + capture_time_ms_last_sent_(0) { UpdateBytesPerInterval(kMinPacketLimitMs); } @@ -113,6 +113,11 @@ bool PacedSender::SendPacket(Priority priority, uint32_t ssrc, Packet(ssrc, sequence_number, capture_time_ms, bytes)); break; case kNormalPriority: + if (capture_time_ms > capture_time_ms_last_queued_) { + capture_time_ms_last_queued_ = capture_time_ms; + TRACE_EVENT_ASYNC_BEGIN1("webrtc_rtp", "PacedSend", capture_time_ms, + "capture_time_ms", capture_time_ms); + } case kLowPriority: // Queue the low priority packets in the normal priority queue when we // are paused to avoid starvation. @@ -140,6 +145,11 @@ bool PacedSender::SendPacket(Priority priority, uint32_t ssrc, UpdateState(bytes); return true; // We can send now. } + if (capture_time_ms > capture_time_ms_last_queued_) { + capture_time_ms_last_queued_ = capture_time_ms; + TRACE_EVENT_ASYNC_BEGIN1("webrtc_rtp", "PacedSend", capture_time_ms, + "capture_time_ms", capture_time_ms); + } normal_priority_packets_.push_back( Packet(ssrc, sequence_number, capture_time_ms, bytes)); return false; @@ -204,7 +214,18 @@ int32_t PacedSender::Process() { uint32_t ssrc; uint16_t sequence_number; int64_t capture_time_ms; - while (GetNextPacket(&ssrc, &sequence_number, &capture_time_ms)) { + Priority priority; + bool last_packet; + while (GetNextPacket(&ssrc, &sequence_number, &capture_time_ms, + &priority, &last_packet)) { + if (priority == kNormalPriority) { + if (capture_time_ms > capture_time_ms_last_sent_) { + capture_time_ms_last_sent_ = capture_time_ms; + } else if (capture_time_ms == capture_time_ms_last_sent_ && + last_packet) { + TRACE_EVENT_ASYNC_END0("webrtc_rtp", "PacedSend", capture_time_ms); + } + } critsect_->Leave(); callback_->TimeToSendPacket(ssrc, sequence_number, capture_time_ms); critsect_->Enter(); @@ -229,10 +250,10 @@ void PacedSender::UpdateBytesPerInterval(uint32_t delta_time_ms) { if (bytes_remaining_interval_ < 0) { // We overused last interval, compensate this interval. - bytes_remaining_interval_ += kBytesPerIntervalMargin * bytes_per_interval; + bytes_remaining_interval_ += pace_multiplier_ * bytes_per_interval; } else { // If we underused last interval we can't use it this interval. - bytes_remaining_interval_ = kBytesPerIntervalMargin * bytes_per_interval; + bytes_remaining_interval_ = pace_multiplier_ * bytes_per_interval; } if (padding_bytes_remaining_interval_ < 0) { // We overused last interval, compensate this interval. @@ -245,51 +266,60 @@ void PacedSender::UpdateBytesPerInterval(uint32_t delta_time_ms) { // MUST have critsect_ when calling. bool PacedSender::GetNextPacket(uint32_t* ssrc, uint16_t* sequence_number, - int64_t* capture_time_ms) { + int64_t* capture_time_ms, Priority* priority, + bool* last_packet) { if (bytes_remaining_interval_ <= 0) { // All bytes consumed for this interval. // Check if we have not sent in a too long time. if ((TickTime::Now() - time_last_send_).Milliseconds() > kMaxQueueTimeWithoutSendingMs) { if (!high_priority_packets_.empty()) { + *priority = kHighPriority; GetNextPacketFromList(&high_priority_packets_, ssrc, sequence_number, - capture_time_ms); + capture_time_ms, last_packet); return true; } if (!normal_priority_packets_.empty()) { + *priority = kNormalPriority; GetNextPacketFromList(&normal_priority_packets_, ssrc, sequence_number, - capture_time_ms); + capture_time_ms, last_packet); return true; } } return false; } if (!high_priority_packets_.empty()) { + *priority = kHighPriority; GetNextPacketFromList(&high_priority_packets_, ssrc, sequence_number, - capture_time_ms); + capture_time_ms, last_packet); return true; } if (!normal_priority_packets_.empty()) { + *priority = kNormalPriority; GetNextPacketFromList(&normal_priority_packets_, ssrc, sequence_number, - capture_time_ms); + capture_time_ms, last_packet); return true; } if (!low_priority_packets_.empty()) { + *priority = kLowPriority; GetNextPacketFromList(&low_priority_packets_, ssrc, sequence_number, - capture_time_ms); + capture_time_ms, last_packet); return true; } return false; } void PacedSender::GetNextPacketFromList(PacketList* list, - uint32_t* ssrc, uint16_t* sequence_number, int64_t* capture_time_ms) { + uint32_t* ssrc, uint16_t* sequence_number, int64_t* capture_time_ms, + bool* last_packet) { Packet packet = list->front(); UpdateState(packet.bytes_); *sequence_number = packet.sequence_number_; *ssrc = packet.ssrc_; *capture_time_ms = packet.capture_time_ms_; list->pop_front(); + *last_packet = list->empty() || + list->front().capture_time_ms_ > *capture_time_ms; } // MUST have critsect_ when calling. diff --git a/webrtc/modules/pacing/paced_sender_unittest.cc b/webrtc/modules/pacing/paced_sender_unittest.cc index a0adceefd..247366c6b 100644 --- a/webrtc/modules/pacing/paced_sender_unittest.cc +++ b/webrtc/modules/pacing/paced_sender_unittest.cc @@ -19,6 +19,7 @@ namespace webrtc { namespace test { static const int kTargetBitrate = 800; +static const float kPaceMultiplier = 1.5f; class MockPacedSenderCallback : public PacedSender::Callback { public: @@ -33,7 +34,8 @@ class PacedSenderTest : public ::testing::Test { PacedSenderTest() { TickTime::UseFakeClock(123456); // Need to initialize PacedSender after we initialize clock. - send_bucket_.reset(new PacedSender(&callback_, kTargetBitrate)); + send_bucket_.reset(new PacedSender(&callback_, kTargetBitrate, + kPaceMultiplier)); send_bucket_->SetStatus(true); } MockPacedSenderCallback callback_; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index e74465f89..1fd8717b9 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -491,6 +491,10 @@ int32_t RTPSender::ReSendPacket(uint16_t packet_id, uint32_t min_resend_time) { // re-transmit and not new payload data. } + TRACE_EVENT_INSTANT2("webrtc_rtp", "RTPSender::ReSendPacket", + "timestamp", rtp_header.header.timestamp, + "seqnum", rtp_header.header.sequenceNumber); + if (paced_sender_) { if (!paced_sender_->SendPacket(PacedSender::kHighPriority, rtp_header.header.ssrc, @@ -503,10 +507,6 @@ int32_t RTPSender::ReSendPacket(uint16_t packet_id, uint32_t min_resend_time) { } } - TRACE_EVENT_INSTANT2("webrtc_rtp", "RTPSender::ReSendPacket", - "timestamp", rtp_header.header.timestamp, - "seqnum", rtp_header.header.sequenceNumber); - if (SendPacketToNetwork(buffer_to_send_ptr, length)) { return 0; } @@ -518,6 +518,8 @@ bool RTPSender::SendPacketToNetwork(const uint8_t *packet, uint32_t size) { if (transport_) { bytes_sent = transport_->SendPacket(id_, packet, size); } + TRACE_EVENT_INSTANT2("webrtc_rtp", "RTPSender::SendPacketToNetwork", + "size", size, "sent", bytes_sent); // TODO(pwesin): Add a separate bitrate for sent bitrate after pacer. if (bytes_sent <= 0) { WEBRTC_TRACE(kTraceWarning, kTraceRtpRtcp, id_, diff --git a/webrtc/video_engine/vie_encoder.cc b/webrtc/video_engine/vie_encoder.cc index 3bc52080e..f75ca1225 100644 --- a/webrtc/video_engine/vie_encoder.cc +++ b/webrtc/video_engine/vie_encoder.cc @@ -33,6 +33,17 @@ namespace webrtc { // Pace in kbits/s until we receive first estimate. static const int kInitialPace = 2000; +// Pacing-rate relative to our target send rate. +// Multiplicative factor that is applied to the target bitrate to calculate the +// number of bytes that can be transmitted per interval. +// Increasing this factor will result in lower delays in cases of bitrate +// overshoots from the encoder. +static const float kPaceMultiplier = 2.5f; + +// Margin on when we pause the encoder when the pacing buffer overflows relative +// to the configured buffer delay. +static const float kEncoderPausePacerMargin = 2.0f; + // Don't stop the encoder unless the delay is above this configured value. static const int kMinPacingDelayMs = 200; @@ -106,6 +117,7 @@ ViEEncoder::ViEEncoder(int32_t engine_id, target_delay_ms_(0), network_is_transmitting_(true), encoder_paused_(false), + encoder_paused_and_dropped_frame_(false), channels_dropping_delta_frames_(0), drop_next_frame_(false), fec_enabled_(false), @@ -131,7 +143,8 @@ ViEEncoder::ViEEncoder(int32_t engine_id, default_rtp_rtcp_.reset(RtpRtcp::CreateRtpRtcp(configuration)); bitrate_observer_.reset(new ViEBitrateObserver(this)); pacing_callback_.reset(new ViEPacedSenderCallback(this)); - paced_sender_.reset(new PacedSender(pacing_callback_.get(), kInitialPace)); + paced_sender_.reset( + new PacedSender(pacing_callback_.get(), kInitialPace, kPaceMultiplier)); } bool ViEEncoder::Init() { @@ -485,7 +498,8 @@ bool ViEEncoder::EncoderPaused() const { // TODO(pwestin): Workaround until nack is configured as a time and not // number of packets. return paced_sender_->QueueInMs() >= - std::max(target_delay_ms_, kMinPacingDelayMs); + std::max(static_cast(target_delay_ms_ * kEncoderPausePacerMargin), + kMinPacingDelayMs); } return !network_is_transmitting_; } @@ -508,10 +522,22 @@ void ViEEncoder::DeliverFrame(int id, video_frame->timestamp()); { CriticalSectionScoped cs(data_cs_.get()); - if (EncoderPaused() || default_rtp_rtcp_->SendingMedia() == false) { + if (default_rtp_rtcp_->SendingMedia() == false) { // We've paused or we have no channels attached, don't encode. return; } + if (EncoderPaused()) { + if (!encoder_paused_and_dropped_frame_) { + TRACE_EVENT_ASYNC_BEGIN0("webrtc", "EncoderPaused", this); + } + encoder_paused_and_dropped_frame_ = true; + return; + } + if (encoder_paused_and_dropped_frame_) { + TRACE_EVENT_ASYNC_END0("webrtc", "EncoderPaused", this); + } + encoder_paused_and_dropped_frame_ = false; + if (drop_next_frame_) { // Drop this frame. WEBRTC_TRACE(webrtc::kTraceStream, diff --git a/webrtc/video_engine/vie_encoder.h b/webrtc/video_engine/vie_encoder.h index b17bafc62..f9dd0bd81 100644 --- a/webrtc/video_engine/vie_encoder.h +++ b/webrtc/video_engine/vie_encoder.h @@ -195,6 +195,7 @@ class ViEEncoder int target_delay_ms_; bool network_is_transmitting_; bool encoder_paused_; + bool encoder_paused_and_dropped_frame_; std::map time_last_intra_request_ms_; int32_t channels_dropping_delta_frames_; bool drop_next_frame_;