diff --git a/webrtc/audio/audio_send_stream.cc b/webrtc/audio/audio_send_stream.cc index 6364202aa6..06c660e499 100644 --- a/webrtc/audio/audio_send_stream.cc +++ b/webrtc/audio/audio_send_stream.cc @@ -19,6 +19,7 @@ #include "webrtc/base/event.h" #include "webrtc/base/logging.h" #include "webrtc/base/task_queue.h" +#include "webrtc/base/timeutils.h" #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" #include "webrtc/modules/congestion_controller/include/send_side_congestion_controller.h" #include "webrtc/modules/pacing/paced_sender.h" @@ -40,6 +41,11 @@ bool IsCodec(const webrtc::CodecInst& codec, const char* ref_name) { } // namespace namespace internal { +// TODO(elad.alon): Subsequent CL will make these values experiment-dependent. +constexpr size_t kPacketLossTrackerMaxWindowSizeMs = 15000; +constexpr size_t kPacketLossRateMinNumAckedPackets = 50; +constexpr size_t kRecoverablePacketLossRateMinNumAckedPairs = 40; + AudioSendStream::AudioSendStream( const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, @@ -53,7 +59,10 @@ AudioSendStream::AudioSendStream( config_(config), audio_state_(audio_state), bitrate_allocator_(bitrate_allocator), - send_side_cc_(send_side_cc) { + send_side_cc_(send_side_cc), + packet_loss_tracker_(kPacketLossTrackerMaxWindowSizeMs, + kPacketLossRateMinNumAckedPackets, + kRecoverablePacketLossRateMinNumAckedPairs) { LOG(LS_INFO) << "AudioSendStream: " << config_.ToString(); RTC_DCHECK_NE(config_.voe_channel_id, -1); RTC_DCHECK(audio_state_.get()); @@ -72,6 +81,7 @@ AudioSendStream::AudioSendStream( config_.rtp.nack.rtp_history_ms / 20); channel_proxy_->RegisterExternalTransport(config.send_transport); + send_side_cc_->RegisterPacketFeedbackObserver(this); for (const auto& extension : config.rtp.extensions) { if (extension.uri == RtpExtension::kAudioLevelUri) { @@ -91,11 +101,14 @@ AudioSendStream::AudioSendStream( if (!SetupSendCodec()) { LOG(LS_ERROR) << "Failed to set up send codec state."; } + + pacer_thread_checker_.DetachFromThread(); } AudioSendStream::~AudioSendStream() { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); LOG(LS_INFO) << "~AudioSendStream: " << config_.ToString(); + send_side_cc_->DeRegisterPacketFeedbackObserver(this); channel_proxy_->DeRegisterExternalTransport(); channel_proxy_->ResetCongestionControlObjects(); channel_proxy_->SetRtcEventLog(nullptr); @@ -103,7 +116,7 @@ AudioSendStream::~AudioSendStream() { } void AudioSendStream::Start() { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); if (config_.min_bitrate_bps != -1 && config_.max_bitrate_bps != -1) { RTC_DCHECK_GE(config_.max_bitrate_bps, config_.min_bitrate_bps); rtc::Event thread_sync_event(false /* manual_reset */, false); @@ -123,7 +136,7 @@ void AudioSendStream::Start() { } void AudioSendStream::Stop() { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); rtc::Event thread_sync_event(false /* manual_reset */, false); worker_queue_->PostTask([this, &thread_sync_event] { bitrate_allocator_->RemoveObserver(this); @@ -141,19 +154,19 @@ void AudioSendStream::Stop() { bool AudioSendStream::SendTelephoneEvent(int payload_type, int payload_frequency, int event, int duration_ms) { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); return channel_proxy_->SetSendTelephoneEventPayloadType(payload_type, payload_frequency) && channel_proxy_->SendTelephoneEventOutband(event, duration_ms); } void AudioSendStream::SetMuted(bool muted) { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); channel_proxy_->SetInputMute(muted); } webrtc::AudioSendStream::Stats AudioSendStream::GetStats() const { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); webrtc::AudioSendStream::Stats stats; stats.local_ssrc = config_.rtp.ssrc; @@ -217,14 +230,14 @@ webrtc::AudioSendStream::Stats AudioSendStream::GetStats() const { } void AudioSendStream::SignalNetworkState(NetworkState state) { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); } bool AudioSendStream::DeliverRtcp(const uint8_t* packet, size_t length) { // TODO(solenberg): Tests call this function on a network thread, libjingle // calls on the worker thread. We should move towards always using a network // thread. Then this check can be enabled. - // RTC_DCHECK(!thread_checker_.CalledOnValidThread()); + // RTC_DCHECK(!worker_thread_checker_.CalledOnValidThread()); return channel_proxy_->ReceivedRTCPPacket(packet, length); } @@ -247,13 +260,43 @@ uint32_t AudioSendStream::OnBitrateUpdated(uint32_t bitrate_bps, return 0; } +void AudioSendStream::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) { + RTC_DCHECK(pacer_thread_checker_.CalledOnValidThread()); + // Only packets that belong to this stream are of interest. + if (ssrc == config_.rtp.ssrc) { + rtc::CritScope lock(&packet_loss_tracker_cs_); + // TODO(elad.alon): This function call could potentially reset the window, + // setting both PLR and RPLR to unknown. Consider (during upcoming + // refactoring) passing an indication of such an event. + packet_loss_tracker_.OnPacketAdded(seq_num, rtc::TimeMillis()); + } +} + +void AudioSendStream::OnPacketFeedbackVector( + const std::vector& packet_feedback_vector) { + // TODO(elad.alon): This fails in UT; fix and uncomment. + // RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); + rtc::Optional plr; + { + rtc::CritScope lock(&packet_loss_tracker_cs_); + packet_loss_tracker_.OnPacketFeedbackVector(packet_feedback_vector); + plr = packet_loss_tracker_.GetPacketLossRate(); + } + // TODO(elad.alon): If PLR goes back to unknown, no indication is given that + // the previously sent value is no longer relevant. This will be taken care + // of with some refactoring which is now being done. + if (plr) { + channel_proxy_->OnTwccBasedUplinkPacketLossRate(*plr); + } +} + const webrtc::AudioSendStream::Config& AudioSendStream::config() const { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); return config_; } void AudioSendStream::SetTransportOverhead(int transport_overhead_per_packet) { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); send_side_cc_->SetTransportOverhead(transport_overhead_per_packet); channel_proxy_->SetTransportOverhead(transport_overhead_per_packet); } diff --git a/webrtc/audio/audio_send_stream.h b/webrtc/audio/audio_send_stream.h index 436c49824c..f50f7c4d02 100644 --- a/webrtc/audio/audio_send_stream.h +++ b/webrtc/audio/audio_send_stream.h @@ -12,12 +12,15 @@ #define WEBRTC_AUDIO_AUDIO_SEND_STREAM_H_ #include +#include #include "webrtc/base/constructormagic.h" #include "webrtc/base/thread_checker.h" #include "webrtc/call/audio_send_stream.h" #include "webrtc/call/audio_state.h" #include "webrtc/call/bitrate_allocator.h" +#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "webrtc/voice_engine/transport_feedback_packet_loss_tracker.h" namespace webrtc { class SendSideCongestionController; @@ -33,7 +36,8 @@ class ChannelProxy; namespace internal { class AudioSendStream final : public webrtc::AudioSendStream, - public webrtc::BitrateAllocatorObserver { + public webrtc::BitrateAllocatorObserver, + public webrtc::PacketFeedbackObserver { public: AudioSendStream(const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, @@ -62,6 +66,11 @@ class AudioSendStream final : public webrtc::AudioSendStream, int64_t rtt, int64_t probing_interval_ms) override; + // From PacketFeedbackObserver. + void OnPacketAdded(uint32_t ssrc, uint16_t seq_num) override; + void OnPacketFeedbackVector( + const std::vector& packet_feedback_vector) override; + const webrtc::AudioSendStream::Config& config() const; void SetTransportOverhead(int transport_overhead_per_packet); @@ -70,7 +79,8 @@ class AudioSendStream final : public webrtc::AudioSendStream, bool SetupSendCodec(); - rtc::ThreadChecker thread_checker_; + rtc::ThreadChecker worker_thread_checker_; + rtc::ThreadChecker pacer_thread_checker_; rtc::TaskQueue* worker_queue_; const webrtc::AudioSendStream::Config config_; rtc::scoped_refptr audio_state_; @@ -80,6 +90,10 @@ class AudioSendStream final : public webrtc::AudioSendStream, SendSideCongestionController* const send_side_cc_; std::unique_ptr bandwidth_observer_; + rtc::CriticalSection packet_loss_tracker_cs_; + TransportFeedbackPacketLossTracker packet_loss_tracker_ + GUARDED_BY(&packet_loss_tracker_cs_); + RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(AudioSendStream); }; } // namespace internal diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index 9b836be348..62772b7f13 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -106,10 +106,11 @@ void CongestionController::Process() { receive_side_cc_.Process(); } -void CongestionController::AddPacket(uint16_t sequence_number, +void CongestionController::AddPacket(uint32_t ssrc, + uint16_t sequence_number, size_t length, const PacedPacketInfo& pacing_info) { - send_side_cc_.AddPacket(sequence_number, length, pacing_info); + send_side_cc_.AddPacket(ssrc, sequence_number, length, pacing_info); } void CongestionController::OnTransportFeedback( diff --git a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc index 8073ae1557..56219363fd 100644 --- a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc +++ b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc @@ -79,7 +79,8 @@ class CongestionControllerTest : public ::testing::Test { } void OnSentPacket(const PacketFeedback& packet_feedback) { - controller_->AddPacket(packet_feedback.sequence_number, + constexpr uint32_t ssrc = 0; + controller_->AddPacket(ssrc, packet_feedback.sequence_number, packet_feedback.payload_size, packet_feedback.pacing_info); controller_->OnSentPacket(rtc::SentPacket(packet_feedback.sequence_number, diff --git a/webrtc/modules/congestion_controller/include/congestion_controller.h b/webrtc/modules/congestion_controller/include/congestion_controller.h index e962f9fda2..be2d68d901 100644 --- a/webrtc/modules/congestion_controller/include/congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/congestion_controller.h @@ -119,7 +119,8 @@ class CongestionController : public CallStatsObserver, void Process() override; // Implements TransportFeedbackObserver. - void AddPacket(uint16_t sequence_number, + void AddPacket(uint32_t ssrc, + uint16_t sequence_number, size_t length, const PacedPacketInfo& pacing_info) override; void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override; diff --git a/webrtc/modules/congestion_controller/include/send_side_congestion_controller.h b/webrtc/modules/congestion_controller/include/send_side_congestion_controller.h index 9aa4727196..8130feb01e 100644 --- a/webrtc/modules/congestion_controller/include/send_side_congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/send_side_congestion_controller.h @@ -67,6 +67,9 @@ class SendSideCongestionController : public CallStatsObserver, std::unique_ptr pacer); virtual ~SendSideCongestionController(); + void RegisterPacketFeedbackObserver(PacketFeedbackObserver* observer); + void DeRegisterPacketFeedbackObserver(PacketFeedbackObserver* observer); + virtual void SetBweBitrates(int min_bitrate_bps, int start_bitrate_bps, int max_bitrate_bps); @@ -111,7 +114,8 @@ class SendSideCongestionController : public CallStatsObserver, void Process() override; // Implements TransportFeedbackObserver. - void AddPacket(uint16_t sequence_number, + void AddPacket(uint32_t ssrc, + uint16_t sequence_number, size_t length, const PacedPacketInfo& pacing_info) override; void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override; diff --git a/webrtc/modules/congestion_controller/send_side_congestion_controller.cc b/webrtc/modules/congestion_controller/send_side_congestion_controller.cc index f3cbed9652..782c060f00 100644 --- a/webrtc/modules/congestion_controller/send_side_congestion_controller.cc +++ b/webrtc/modules/congestion_controller/send_side_congestion_controller.cc @@ -83,6 +83,16 @@ SendSideCongestionController::SendSideCongestionController( SendSideCongestionController::~SendSideCongestionController() {} +void SendSideCongestionController::RegisterPacketFeedbackObserver( + PacketFeedbackObserver* observer) { + transport_feedback_adapter_.RegisterPacketFeedbackObserver(observer); +} + +void SendSideCongestionController::DeRegisterPacketFeedbackObserver( + PacketFeedbackObserver* observer) { + transport_feedback_adapter_.DeRegisterPacketFeedbackObserver(observer); +} + void SendSideCongestionController::SetBweBitrates(int min_bitrate_bps, int start_bitrate_bps, int max_bitrate_bps) { @@ -203,10 +213,12 @@ void SendSideCongestionController::Process() { } void SendSideCongestionController::AddPacket( + uint32_t ssrc, uint16_t sequence_number, size_t length, const PacedPacketInfo& pacing_info) { - transport_feedback_adapter_.AddPacket(sequence_number, length, pacing_info); + transport_feedback_adapter_.AddPacket(ssrc, sequence_number, length, + pacing_info); } void SendSideCongestionController::OnTransportFeedback( diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter.cc b/webrtc/modules/congestion_controller/transport_feedback_adapter.cc index ddffa72196..9fb1af59a2 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter.cc +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter.cc @@ -36,19 +36,49 @@ TransportFeedbackAdapter::TransportFeedbackAdapter(const Clock* clock) local_net_id_(0), remote_net_id_(0) {} -TransportFeedbackAdapter::~TransportFeedbackAdapter() {} +TransportFeedbackAdapter::~TransportFeedbackAdapter() { + RTC_DCHECK(observers_.empty()); +} -void TransportFeedbackAdapter::AddPacket(uint16_t sequence_number, +void TransportFeedbackAdapter::RegisterPacketFeedbackObserver( + PacketFeedbackObserver* observer) { + rtc::CritScope cs(&observers_lock_); + RTC_DCHECK(observer); + RTC_DCHECK(std::find(observers_.begin(), observers_.end(), observer) == + observers_.end()); + observers_.push_back(observer); +} + +void TransportFeedbackAdapter::DeRegisterPacketFeedbackObserver( + PacketFeedbackObserver* observer) { + rtc::CritScope cs(&observers_lock_); + RTC_DCHECK(observer); + const auto it = std::find(observers_.begin(), observers_.end(), observer); + RTC_DCHECK(it != observers_.end()); + observers_.erase(it); +} + +void TransportFeedbackAdapter::AddPacket(uint32_t ssrc, + uint16_t sequence_number, size_t length, const PacedPacketInfo& pacing_info) { - rtc::CritScope cs(&lock_); - if (send_side_bwe_with_overhead_) { - length += transport_overhead_bytes_per_packet_; + { + rtc::CritScope cs(&lock_); + if (send_side_bwe_with_overhead_) { + length += transport_overhead_bytes_per_packet_; + } + const int64_t creation_time_ms = clock_->TimeInMilliseconds(); + send_time_history_.AddAndRemoveOld( + PacketFeedback(creation_time_ms, sequence_number, length, local_net_id_, + remote_net_id_, pacing_info)); + } + + { + rtc::CritScope cs(&observers_lock_); + for (auto observer : observers_) { + observer->OnPacketAdded(ssrc, sequence_number); + } } - const int64_t creation_time_ms = clock_->TimeInMilliseconds(); - send_time_history_.AddAndRemoveOld( - PacketFeedback(creation_time_ms, sequence_number, length, local_net_id_, - remote_net_id_, pacing_info)); } void TransportFeedbackAdapter::OnSentPacket(uint16_t sequence_number, @@ -154,6 +184,12 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( void TransportFeedbackAdapter::OnTransportFeedback( const rtcp::TransportFeedback& feedback) { last_packet_feedback_vector_ = GetPacketFeedbackVector(feedback); + { + rtc::CritScope cs(&observers_lock_); + for (auto observer : observers_) { + observer->OnPacketFeedbackVector(last_packet_feedback_vector_); + } + } } std::vector diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter.h b/webrtc/modules/congestion_controller/transport_feedback_adapter.h index 616bebe00a..2e70e448da 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter.h +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter.h @@ -21,6 +21,8 @@ namespace webrtc { +class PacketFeedbackObserver; + namespace rtcp { class TransportFeedback; } // namespace rtcp @@ -30,7 +32,11 @@ class TransportFeedbackAdapter { explicit TransportFeedbackAdapter(const Clock* clock); virtual ~TransportFeedbackAdapter(); - void AddPacket(uint16_t sequence_number, + void RegisterPacketFeedbackObserver(PacketFeedbackObserver* observer); + void DeRegisterPacketFeedbackObserver(PacketFeedbackObserver* observer); + + void AddPacket(uint32_t ssrc, + uint16_t sequence_number, size_t length, const PacedPacketInfo& pacing_info); void OnSentPacket(uint16_t sequence_number, int64_t send_time_ms); @@ -57,8 +63,11 @@ class TransportFeedbackAdapter { int64_t current_offset_ms_; int64_t last_timestamp_us_; std::vector last_packet_feedback_vector_; - uint16_t local_net_id_; - uint16_t remote_net_id_; + uint16_t local_net_id_ GUARDED_BY(&lock_); + uint16_t remote_net_id_ GUARDED_BY(&lock_); + + rtc::CriticalSection observers_lock_; + std::vector observers_ GUARDED_BY(&observers_lock_); }; } // namespace webrtc diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc b/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc index 22e8b59b95..97b7f5f2a5 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc @@ -38,6 +38,13 @@ const PacedPacketInfo kPacingInfo4(4, 22, 10000); namespace test { +class MockPacketFeedbackObserver : public webrtc::PacketFeedbackObserver { + public: + MOCK_METHOD2(OnPacketAdded, void(uint32_t ssrc, uint16_t seq_num)); + MOCK_METHOD1(OnPacketFeedbackVector, + void(const std::vector& packet_feedback_vector)); +}; + class TransportFeedbackAdapterTest : public ::testing::Test { public: TransportFeedbackAdapterTest() : clock_(0) {} @@ -58,17 +65,75 @@ class TransportFeedbackAdapterTest : public ::testing::Test { int64_t now_ms) {} void OnSentPacket(const PacketFeedback& packet_feedback) { - adapter_->AddPacket(packet_feedback.sequence_number, + adapter_->AddPacket(kSsrc, packet_feedback.sequence_number, packet_feedback.payload_size, packet_feedback.pacing_info); adapter_->OnSentPacket(packet_feedback.sequence_number, packet_feedback.send_time_ms); } + static constexpr uint32_t kSsrc = 8492; + SimulatedClock clock_; std::unique_ptr adapter_; }; +TEST_F(TransportFeedbackAdapterTest, ObserverSanity) { + MockPacketFeedbackObserver mock; + adapter_->RegisterPacketFeedbackObserver(&mock); + + const std::vector packets = { + PacketFeedback(100, 200, 0, 1000, kPacingInfo0), + PacketFeedback(110, 210, 1, 2000, kPacingInfo0), + PacketFeedback(120, 220, 2, 3000, kPacingInfo0) + }; + + rtcp::TransportFeedback feedback; + feedback.SetBase(packets[0].sequence_number, + packets[0].arrival_time_ms * 1000); + + for (const PacketFeedback& packet : packets) { + EXPECT_CALL(mock, OnPacketAdded(kSsrc, packet.sequence_number)).Times(1); + OnSentPacket(packet); + EXPECT_TRUE(feedback.AddReceivedPacket(packet.sequence_number, + packet.arrival_time_ms * 1000)); + } + + EXPECT_CALL(mock, OnPacketFeedbackVector(_)).Times(1); + adapter_->OnTransportFeedback(feedback); + + adapter_->DeRegisterPacketFeedbackObserver(&mock); + + // After deregistration, the observer no longers gets indications. + EXPECT_CALL(mock, OnPacketAdded(_, _)).Times(0); + const PacketFeedback new_packet(130, 230, 3, 4000, kPacingInfo0); + OnSentPacket(new_packet); + + rtcp::TransportFeedback second_feedback; + second_feedback.SetBase(new_packet.sequence_number, + new_packet.arrival_time_ms * 1000); + EXPECT_TRUE(feedback.AddReceivedPacket(new_packet.sequence_number, + new_packet.arrival_time_ms * 1000)); + EXPECT_CALL(mock, OnPacketFeedbackVector(_)).Times(0); + adapter_->OnTransportFeedback(second_feedback); +} + +#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) +TEST_F(TransportFeedbackAdapterTest, ObserverDoubleRegistrationDeathTest) { + MockPacketFeedbackObserver mock; + adapter_->RegisterPacketFeedbackObserver(&mock); + EXPECT_DEATH(adapter_->RegisterPacketFeedbackObserver(&mock), ""); + adapter_->DeRegisterPacketFeedbackObserver(&mock); +} + +TEST_F(TransportFeedbackAdapterTest, ObserverMissingDeRegistrationDeathTest) { + MockPacketFeedbackObserver mock; + adapter_->RegisterPacketFeedbackObserver(&mock); + EXPECT_DEATH(adapter_.reset(), ""); + adapter_->DeRegisterPacketFeedbackObserver(&mock); +} +#endif + TEST_F(TransportFeedbackAdapterTest, AdaptsFeedbackAndPopulatesSendTimes) { std::vector packets; packets.push_back(PacketFeedback(100, 200, 0, 1500, kPacingInfo0)); diff --git a/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 7df8a131c6..ddfec4d7cd 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -342,7 +342,8 @@ class TransportFeedbackObserver { virtual ~TransportFeedbackObserver() {} // Note: Transport-wide sequence number as sequence number. - virtual void AddPacket(uint16_t sequence_number, + virtual void AddPacket(uint32_t ssrc, + uint16_t sequence_number, size_t length, const PacedPacketInfo& pacing_info) = 0; @@ -351,6 +352,15 @@ class TransportFeedbackObserver { virtual std::vector GetTransportFeedbackVector() const = 0; }; +class PacketFeedbackObserver { + public: + virtual ~PacketFeedbackObserver() = default; + + virtual void OnPacketAdded(uint32_t ssrc, uint16_t seq_num) = 0; + virtual void OnPacketFeedbackVector( + const std::vector& packet_feedback_vector) = 0; +}; + class RtcpRttStats { public: virtual void OnRttUpdate(int64_t rtt) = 0; diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index 4b0255f23b..d419caffeb 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -73,8 +73,9 @@ class MockRtcpCallbackImpl : public RtcpStatisticsCallback { class MockTransportFeedbackObserver : public TransportFeedbackObserver { public: - MOCK_METHOD2(AddPacket, void(uint16_t, size_t)); - MOCK_METHOD3(AddPacket, void(uint16_t, size_t, const PacedPacketInfo&)); + MOCK_METHOD3(AddPacket, void(uint32_t, uint16_t, size_t)); + MOCK_METHOD4(AddPacket, + void(uint32_t, uint16_t, size_t, const PacedPacketInfo&)); MOCK_METHOD1(OnTransportFeedback, void(const rtcp::TransportFeedback&)); MOCK_CONST_METHOD0(GetTransportFeedbackVector, std::vector()); }; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index 2fd8b3eff5..52b0891ea7 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -1256,7 +1256,7 @@ void RTPSender::AddPacketToTransportFeedback( } if (transport_feedback_observer_) { - transport_feedback_observer_->AddPacket(packet_id, packet_size, + transport_feedback_observer_->AddPacket(SSRC(), packet_id, packet_size, pacing_info); } } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc index fed52ee190..6539f736dc 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -126,7 +126,8 @@ class MockSendPacketObserver : public SendPacketObserver { class MockTransportFeedbackObserver : public TransportFeedbackObserver { public: - MOCK_METHOD3(AddPacket, void(uint16_t, size_t, const PacedPacketInfo&)); + MOCK_METHOD4(AddPacket, + void(uint32_t, uint16_t, size_t, const PacedPacketInfo&)); MOCK_METHOD1(OnTransportFeedback, void(const rtcp::TransportFeedback&)); MOCK_CONST_METHOD0(GetTransportFeedbackVector, std::vector()); }; @@ -355,7 +356,7 @@ TEST_F(RtpSenderTestWithoutPacer, SendsPacketsWithTransportSequenceNumber) { .Times(1); EXPECT_CALL( feedback_observer_, - AddPacket(kTransportSequenceNumber, + AddPacket(rtp_sender_->SSRC(), kTransportSequenceNumber, sizeof(kPayloadData) + kGenericHeaderLength, PacedPacketInfo())) .Times(1); @@ -406,7 +407,7 @@ TEST_F(RtpSenderTest, SendsPacketsWithTransportSequenceNumber) { .Times(1); EXPECT_CALL( feedback_observer_, - AddPacket(kTransportSequenceNumber, + AddPacket(rtp_sender_->SSRC(), kTransportSequenceNumber, sizeof(kPayloadData) + kGenericHeaderLength, PacedPacketInfo())) .Times(1); @@ -1487,7 +1488,7 @@ TEST_F(RtpSenderTest, AddOverheadToTransportFeedbackObserver) { EXPECT_CALL(seq_num_allocator_, AllocateSequenceNumber()) .WillOnce(testing::Return(kTransportSequenceNumber)); EXPECT_CALL(feedback_observer_, - AddPacket(kTransportSequenceNumber, + AddPacket(rtp_sender_->SSRC(), kTransportSequenceNumber, sizeof(kPayloadData) + kGenericHeaderLength + kRtpOverheadBytesPerPacket, PacedPacketInfo())) diff --git a/webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc b/webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc index bcb075d046..cf19573d9a 100644 --- a/webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc +++ b/webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc @@ -245,7 +245,7 @@ bool FuzzTransportFeedbackBlock( if (!may_continue) { return false; } - tracker->OnNewTransportFeedbackVector(feedback_vector); + tracker->OnPacketFeedbackVector(feedback_vector); tracker->Validate(); } diff --git a/webrtc/test/mock_voe_channel_proxy.h b/webrtc/test/mock_voe_channel_proxy.h index 9aeb2d1b3a..24adcc285d 100644 --- a/webrtc/test/mock_voe_channel_proxy.h +++ b/webrtc/test/mock_voe_channel_proxy.h @@ -87,6 +87,7 @@ class MockVoEChannelProxy : public voe::ChannelProxy { MOCK_METHOD1(SetSendCodec, bool(const CodecInst& codec_inst)); MOCK_METHOD2(SetSendCNPayloadType, bool(int type, PayloadFrequencies frequency)); + MOCK_METHOD1(OnTwccBasedUplinkPacketLossRate, void(float packet_loss_rate)); }; } // namespace test } // namespace webrtc diff --git a/webrtc/tools/event_log_visualizer/analyzer.cc b/webrtc/tools/event_log_visualizer/analyzer.cc index fa772d705f..c986ff7db9 100644 --- a/webrtc/tools/event_log_visualizer/analyzer.cc +++ b/webrtc/tools/event_log_visualizer/analyzer.cc @@ -1070,7 +1070,8 @@ void EventLogAnalyzer::CreateBweSimulationGraph(Plot* plot) { const LoggedRtpPacket& rtp = *rtp_iterator->second; if (rtp.header.extension.hasTransportSequenceNumber) { RTC_DCHECK(rtp.header.extension.hasTransportSequenceNumber); - cc.AddPacket(rtp.header.extension.transportSequenceNumber, + cc.AddPacket(rtp.header.ssrc, + rtp.header.extension.transportSequenceNumber, rtp.total_length, PacedPacketInfo()); rtc::SentPacket sent_packet( rtp.header.extension.transportSequenceNumber, rtp.timestamp / 1000); @@ -1169,7 +1170,8 @@ void EventLogAnalyzer::CreateNetworkDelayFeedbackGraph(Plot* plot) { const LoggedRtpPacket& rtp = *rtp_iterator->second; if (rtp.header.extension.hasTransportSequenceNumber) { RTC_DCHECK(rtp.header.extension.hasTransportSequenceNumber); - feedback_adapter.AddPacket(rtp.header.extension.transportSequenceNumber, + feedback_adapter.AddPacket(rtp.header.ssrc, + rtp.header.extension.transportSequenceNumber, rtp.total_length, PacedPacketInfo()); feedback_adapter.OnSentPacket( rtp.header.extension.transportSequenceNumber, rtp.timestamp / 1000); diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc index 0f0831c8b1..7e49e321cf 100644 --- a/webrtc/voice_engine/channel.cc +++ b/webrtc/voice_engine/channel.cc @@ -246,13 +246,14 @@ class TransportFeedbackProxy : public TransportFeedbackObserver { } // Implements TransportFeedbackObserver. - void AddPacket(uint16_t sequence_number, + void AddPacket(uint32_t ssrc, + uint16_t sequence_number, size_t length, const PacedPacketInfo& pacing_info) override { RTC_DCHECK(pacer_thread_.CalledOnValidThread()); rtc::CritScope lock(&crit_); if (feedback_observer_) - feedback_observer_->AddPacket(sequence_number, length, pacing_info); + feedback_observer_->AddPacket(ssrc, sequence_number, length, pacing_info); } void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override { @@ -395,7 +396,7 @@ class VoERtcpObserver : public RtcpBandwidthObserver { (fraction_lost_aggregate + total_number_of_packets / 2) / total_number_of_packets; } - owner_->OnIncomingFractionLoss(weighted_fraction_lost); + owner_->OnUplinkPacketLossRate(weighted_fraction_lost / 255.0f); } private: @@ -902,7 +903,9 @@ Channel::Channel(int32_t channelId, rtp_packet_sender_proxy_(new RtpPacketSenderProxy()), retransmission_rate_limiter_(new RateLimiter(Clock::GetRealTimeClock(), kMaxRetransmissionWindowMs)), - decoder_factory_(config.acm_config.decoder_factory) { + decoder_factory_(config.acm_config.decoder_factory), + // TODO(elad.alon): Subsequent CL experiments with PLR source. + use_twcc_plr_for_ana_(false) { WEBRTC_TRACE(kTraceMemory, kTraceVoice, VoEId(_instanceId, _channelId), "Channel::Channel() - ctor"); AudioCodingModule::Config acm_config(config.acm_config); @@ -1301,10 +1304,23 @@ void Channel::SetBitRate(int bitrate_bps, int64_t probing_interval_ms) { retransmission_rate_limiter_->SetMaxRate(bitrate_bps); } -void Channel::OnIncomingFractionLoss(int fraction_lost) { +void Channel::OnTwccBasedUplinkPacketLossRate(float packet_loss_rate) { + if (!use_twcc_plr_for_ana_) + return; audio_coding_->ModifyEncoder([&](std::unique_ptr* encoder) { - if (*encoder) - (*encoder)->OnReceivedUplinkPacketLossFraction(fraction_lost / 255.0f); + if (*encoder) { + (*encoder)->OnReceivedUplinkPacketLossFraction(packet_loss_rate); + } + }); +} + +void Channel::OnUplinkPacketLossRate(float packet_loss_rate) { + if (use_twcc_plr_for_ana_) + return; + audio_coding_->ModifyEncoder([&](std::unique_ptr* encoder) { + if (*encoder) { + (*encoder)->OnReceivedUplinkPacketLossFraction(packet_loss_rate); + } }); } diff --git a/webrtc/voice_engine/channel.h b/webrtc/voice_engine/channel.h index 68d022d4d1..d24eb5f183 100644 --- a/webrtc/voice_engine/channel.h +++ b/webrtc/voice_engine/channel.h @@ -378,10 +378,15 @@ class Channel // From OverheadObserver in the RTP/RTCP module void OnOverheadChanged(size_t overhead_bytes_per_packet) override; - protected: - void OnIncomingFractionLoss(int fraction_lost); + // The existence of this function alongside OnUplinkPacketLossRate is + // a compromise. We want the encoder to be agnostic of the PLR source, but + // we also don't want it to receive conflicting information from TWCC and + // from RTCP-XR. + void OnTwccBasedUplinkPacketLossRate(float packet_loss_rate); private: + void OnUplinkPacketLossRate(float packet_loss_rate); + bool InputMute() const; bool OnRtpPacketWithHeader(const uint8_t* received_packet, size_t length, @@ -508,6 +513,8 @@ class Channel rtc::scoped_refptr decoder_factory_; rtc::ThreadChecker construction_thread_; + + const bool use_twcc_plr_for_ana_; }; } // namespace voe diff --git a/webrtc/voice_engine/channel_proxy.cc b/webrtc/voice_engine/channel_proxy.cc index 7f6e96c97c..5388b8d3e3 100644 --- a/webrtc/voice_engine/channel_proxy.cc +++ b/webrtc/voice_engine/channel_proxy.cc @@ -367,6 +367,11 @@ bool ChannelProxy::SetSendCNPayloadType(int type, return channel()->SetSendCNPayloadType(type, frequency) == 0; } +void ChannelProxy::OnTwccBasedUplinkPacketLossRate(float packet_loss_rate) { + RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); + channel()->OnTwccBasedUplinkPacketLossRate(packet_loss_rate); +} + Channel* ChannelProxy::channel() const { RTC_DCHECK(channel_owner_.channel()); return channel_owner_.channel(); diff --git a/webrtc/voice_engine/channel_proxy.h b/webrtc/voice_engine/channel_proxy.h index 28d036446a..fd25378af5 100644 --- a/webrtc/voice_engine/channel_proxy.h +++ b/webrtc/voice_engine/channel_proxy.h @@ -116,6 +116,7 @@ class ChannelProxy { virtual bool SetOpusMaxPlaybackRate(int frequency_hz); virtual bool SetSendCodec(const CodecInst& codec_inst); virtual bool SetSendCNPayloadType(int type, PayloadFrequencies frequency); + virtual void OnTwccBasedUplinkPacketLossRate(float packet_loss_rate); private: Channel* channel() const; diff --git a/webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc b/webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc index 6cdeec2f1f..07af861944 100644 --- a/webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc +++ b/webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc @@ -98,7 +98,7 @@ void TransportFeedbackPacketLossTracker::OnPacketAdded(uint16_t seq_num, } } -void TransportFeedbackPacketLossTracker::OnNewTransportFeedbackVector( +void TransportFeedbackPacketLossTracker::OnPacketFeedbackVector( const std::vector& packet_feedback_vector) { for (const PacketFeedback& packet : packet_feedback_vector) { const auto& it = packet_status_window_.find(packet.sequence_number); diff --git a/webrtc/voice_engine/transport_feedback_packet_loss_tracker.h b/webrtc/voice_engine/transport_feedback_packet_loss_tracker.h index 231fb7070a..ea82f74ca5 100644 --- a/webrtc/voice_engine/transport_feedback_packet_loss_tracker.h +++ b/webrtc/voice_engine/transport_feedback_packet_loss_tracker.h @@ -38,7 +38,7 @@ class TransportFeedbackPacketLossTracker final { void OnPacketAdded(uint16_t seq_num, int64_t send_time_ms); - void OnNewTransportFeedbackVector( + void OnPacketFeedbackVector( const std::vector& packet_feedbacks_vector); // Returns the packet loss rate, if the window has enough packet statuses to diff --git a/webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc b/webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc index 1e463dd611..9f7d578a49 100644 --- a/webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc +++ b/webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc @@ -86,7 +86,7 @@ class TransportFeedbackPacketLossTrackerTest ++seq_num; } - tracker->OnNewTransportFeedbackVector(packet_feedback_vector); + tracker->OnPacketFeedbackVector(packet_feedback_vector); tracker->Validate(); }