From 61dfaafcb2d9b182ff3430679ae4a0924f334101 Mon Sep 17 00:00:00 2001 From: Per K Date: Tue, 20 Dec 2022 11:25:31 +0100 Subject: [PATCH] Delete unused feature to cache packets for unsignaled SSRCs. This delete field trial WebRTC-Video-BufferPacketsWithUnknownSsrc. Bug: webrtc:10405 Change-Id: I478c015b359dece6041f2a768d5aa1055235ee6f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/288600 Auto-Submit: Per Kjellander Commit-Queue: Per Kjellander Reviewed-by: Rasmus Brandt Cr-Commit-Position: refs/heads/main@{#38926} --- media/BUILD.gn | 3 - media/engine/unhandled_packets_buffer.cc | 68 ------- media/engine/unhandled_packets_buffer.h | 55 ------ .../unhandled_packets_buffer_unittest.cc | 171 ------------------ media/engine/webrtc_video_engine.cc | 70 +------ media/engine/webrtc_video_engine.h | 11 -- 6 files changed, 5 insertions(+), 373 deletions(-) delete mode 100644 media/engine/unhandled_packets_buffer.cc delete mode 100644 media/engine/unhandled_packets_buffer.h delete mode 100644 media/engine/unhandled_packets_buffer_unittest.cc diff --git a/media/BUILD.gn b/media/BUILD.gn index cc5bbd1d4b..dca97e9e15 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -535,8 +535,6 @@ rtc_library("rtc_audio_video") { "engine/null_webrtc_video_engine.h", "engine/payload_type_mapper.cc", "engine/payload_type_mapper.h", - "engine/unhandled_packets_buffer.cc", - "engine/unhandled_packets_buffer.h", "engine/webrtc_media_engine.cc", "engine/webrtc_media_engine.h", "engine/webrtc_video_engine.cc", @@ -887,7 +885,6 @@ if (rtc_include_tests) { "engine/null_webrtc_video_engine_unittest.cc", "engine/payload_type_mapper_unittest.cc", "engine/simulcast_encoder_adapter_unittest.cc", - "engine/unhandled_packets_buffer_unittest.cc", "engine/webrtc_media_engine_unittest.cc", "engine/webrtc_video_engine_unittest.cc", ] diff --git a/media/engine/unhandled_packets_buffer.cc b/media/engine/unhandled_packets_buffer.cc deleted file mode 100644 index 563712bdf3..0000000000 --- a/media/engine/unhandled_packets_buffer.cc +++ /dev/null @@ -1,68 +0,0 @@ -/* - * Copyright (c) 2019 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "media/engine/unhandled_packets_buffer.h" - -#include "absl/algorithm/container.h" -#include "rtc_base/logging.h" -#include "rtc_base/strings/string_builder.h" - -namespace cricket { - -UnhandledPacketsBuffer::UnhandledPacketsBuffer() { - buffer_.reserve(kMaxStashedPackets); -} - -UnhandledPacketsBuffer::~UnhandledPacketsBuffer() = default; - -// Store packet in buffer. -void UnhandledPacketsBuffer::AddPacket(uint32_t ssrc, - int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - if (buffer_.size() < kMaxStashedPackets) { - buffer_.push_back({ssrc, packet_time_us, packet}); - } else { - RTC_DCHECK_LT(insert_pos_, kMaxStashedPackets); - buffer_[insert_pos_] = {ssrc, packet_time_us, packet}; - } - insert_pos_ = (insert_pos_ + 1) % kMaxStashedPackets; -} - -// Backfill `consumer` with all stored packet related `ssrcs`. -void UnhandledPacketsBuffer::BackfillPackets( - rtc::ArrayView ssrcs, - std::function consumer) { - size_t start; - if (buffer_.size() < kMaxStashedPackets) { - start = 0; - } else { - start = insert_pos_; - } - - std::vector remaining; - remaining.reserve(kMaxStashedPackets); - for (size_t i = 0; i < buffer_.size(); ++i) { - const size_t pos = (i + start) % kMaxStashedPackets; - - // One or maybe 2 ssrcs is expected => loop array instead of more elaborate - // scheme. - const uint32_t ssrc = buffer_[pos].ssrc; - if (absl::c_linear_search(ssrcs, ssrc)) { - consumer(ssrc, buffer_[pos].packet_time_us, buffer_[pos].packet); - } else { - remaining.push_back(buffer_[pos]); - } - } - - insert_pos_ = 0; // insert_pos is only used when buffer is full. - buffer_.swap(remaining); -} - -} // namespace cricket diff --git a/media/engine/unhandled_packets_buffer.h b/media/engine/unhandled_packets_buffer.h deleted file mode 100644 index 63a6195c46..0000000000 --- a/media/engine/unhandled_packets_buffer.h +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright (c) 2019 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#ifndef MEDIA_ENGINE_UNHANDLED_PACKETS_BUFFER_H_ -#define MEDIA_ENGINE_UNHANDLED_PACKETS_BUFFER_H_ - -#include - -#include -#include -#include -#include - -#include "rtc_base/copy_on_write_buffer.h" - -namespace cricket { - -class UnhandledPacketsBuffer { - public: - // Visible for testing. - static constexpr size_t kMaxStashedPackets = 50; - - UnhandledPacketsBuffer(); - ~UnhandledPacketsBuffer(); - - // Store packet in buffer. - void AddPacket(uint32_t ssrc, - int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet); - - // Feed all packets with `ssrcs` into `consumer`. - void BackfillPackets( - rtc::ArrayView ssrcs, - std::function consumer); - - private: - size_t insert_pos_ = 0; - struct PacketWithMetadata { - uint32_t ssrc; - int64_t packet_time_us; - rtc::CopyOnWriteBuffer packet; - }; - std::vector buffer_; -}; - -} // namespace cricket - -#endif // MEDIA_ENGINE_UNHANDLED_PACKETS_BUFFER_H_ diff --git a/media/engine/unhandled_packets_buffer_unittest.cc b/media/engine/unhandled_packets_buffer_unittest.cc deleted file mode 100644 index 11abd86850..0000000000 --- a/media/engine/unhandled_packets_buffer_unittest.cc +++ /dev/null @@ -1,171 +0,0 @@ -/* - * Copyright (c) 2019 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "media/engine/unhandled_packets_buffer.h" - -#include - -#include "absl/memory/memory.h" -#include "test/gmock.h" -#include "test/gtest.h" - -using ::testing::_; - -namespace { - -rtc::CopyOnWriteBuffer Create(int n) { - return rtc::CopyOnWriteBuffer(std::to_string(n)); -} - -constexpr int64_t kPacketTimeUs = 122; - -} // namespace - -namespace cricket { - -TEST(UnhandledPacketsBuffer, NoPackets) { - UnhandledPacketsBuffer buff; - buff.AddPacket(2, kPacketTimeUs, Create(3)); - - std::vector ssrcs = {3}; - std::vector packets; - buff.BackfillPackets(ssrcs, [&packets](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - packets.push_back(packet); - }); - EXPECT_EQ(0u, packets.size()); -} - -TEST(UnhandledPacketsBuffer, OnePacket) { - UnhandledPacketsBuffer buff; - buff.AddPacket(2, kPacketTimeUs, Create(3)); - - std::vector ssrcs = {2}; - std::vector packets; - buff.BackfillPackets(ssrcs, [&packets](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - packets.push_back(packet); - }); - ASSERT_EQ(1u, packets.size()); - EXPECT_EQ(Create(3), packets[0]); -} - -TEST(UnhandledPacketsBuffer, TwoPacketsTwoSsrcs) { - UnhandledPacketsBuffer buff; - buff.AddPacket(2, kPacketTimeUs, Create(3)); - buff.AddPacket(3, kPacketTimeUs, Create(4)); - - std::vector ssrcs = {2, 3}; - std::vector packets; - buff.BackfillPackets(ssrcs, [&packets](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - packets.push_back(packet); - }); - ASSERT_EQ(2u, packets.size()); - EXPECT_EQ(Create(3), packets[0]); - EXPECT_EQ(Create(4), packets[1]); -} - -TEST(UnhandledPacketsBuffer, TwoPacketsTwoSsrcsOneMatch) { - UnhandledPacketsBuffer buff; - buff.AddPacket(2, kPacketTimeUs, Create(3)); - buff.AddPacket(3, kPacketTimeUs, Create(4)); - - std::vector ssrcs = {3}; - buff.BackfillPackets(ssrcs, [](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - EXPECT_EQ(ssrc, 3u); - EXPECT_EQ(Create(4), packet); - }); - - std::vector ssrcs_again = {2}; - buff.BackfillPackets(ssrcs, [](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - EXPECT_EQ(ssrc, 2u); - EXPECT_EQ(Create(3), packet); - }); -} - -TEST(UnhandledPacketsBuffer, Full) { - const size_t cnt = UnhandledPacketsBuffer::kMaxStashedPackets; - UnhandledPacketsBuffer buff; - for (size_t i = 0; i < cnt; i++) { - buff.AddPacket(2, kPacketTimeUs, Create(i)); - } - - std::vector ssrcs = {2}; - std::vector packets; - buff.BackfillPackets(ssrcs, [&packets](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - packets.push_back(packet); - }); - ASSERT_EQ(cnt, packets.size()); - for (size_t i = 0; i < cnt; i++) { - EXPECT_EQ(Create(i), packets[i]); - } - - // Add a packet after backfill and check that it comes back. - buff.AddPacket(23, kPacketTimeUs, Create(1001)); - buff.BackfillPackets(ssrcs, [](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - EXPECT_EQ(ssrc, 23u); - EXPECT_EQ(Create(1001), packet); - }); -} - -TEST(UnhandledPacketsBuffer, Wrap) { - UnhandledPacketsBuffer buff; - size_t cnt = UnhandledPacketsBuffer::kMaxStashedPackets + 10; - for (size_t i = 0; i < cnt; i++) { - buff.AddPacket(2, kPacketTimeUs, Create(i)); - } - - std::vector ssrcs = {2}; - std::vector packets; - buff.BackfillPackets(ssrcs, [&packets](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - packets.push_back(packet); - }); - for (size_t i = 0; i < packets.size(); i++) { - EXPECT_EQ(Create(i + 10), packets[i]); - } - - // Add a packet after backfill and check that it comes back. - buff.AddPacket(23, kPacketTimeUs, Create(1001)); - buff.BackfillPackets(ssrcs, [](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - EXPECT_EQ(ssrc, 23u); - EXPECT_EQ(Create(1001), packet); - }); -} - -TEST(UnhandledPacketsBuffer, Interleaved) { - UnhandledPacketsBuffer buff; - buff.AddPacket(2, kPacketTimeUs, Create(2)); - buff.AddPacket(3, kPacketTimeUs, Create(3)); - - std::vector ssrcs = {2}; - buff.BackfillPackets(ssrcs, [](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - EXPECT_EQ(ssrc, 2u); - EXPECT_EQ(Create(2), packet); - }); - - buff.AddPacket(4, kPacketTimeUs, Create(4)); - - ssrcs = {3}; - buff.BackfillPackets(ssrcs, [](uint32_t ssrc, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - EXPECT_EQ(ssrc, 3u); - EXPECT_EQ(Create(3), packet); - }); -} - -} // namespace cricket diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 34beacdab7..dbf760520b 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -694,12 +694,7 @@ WebRtcVideoChannel::WebRtcVideoChannel( discard_unknown_ssrc_packets_( IsEnabled(call_->trials(), "WebRTC-Video-DiscardPacketsWithUnknownSsrc")), - crypto_options_(crypto_options), - unknown_ssrc_packet_buffer_( - IsEnabled(call_->trials(), - "WebRTC-Video-BufferPacketsWithUnknownSsrc") - ? new UnhandledPacketsBuffer() - : nullptr) { + crypto_options_(crypto_options) { RTC_DCHECK_RUN_ON(&thread_checker_); network_thread_checker_.Detach(); @@ -1478,9 +1473,9 @@ bool WebRtcVideoChannel::AddRecvStream(const StreamParams& sp, if (unsignaled_frame_transformer_ && !config.frame_transformer) config.frame_transformer = unsignaled_frame_transformer_; - receive_streams_[sp.first_ssrc()] = new WebRtcVideoReceiveStream( - this, call_, sp, std::move(config), default_stream, recv_codecs_, - flexfec_config); + receive_streams_[sp.first_ssrc()] = + new WebRtcVideoReceiveStream(call_, sp, std::move(config), default_stream, + recv_codecs_, flexfec_config); return true; } @@ -1723,10 +1718,6 @@ void WebRtcVideoChannel::OnPacketReceived(rtc::CopyOnWriteBuffer packet, absl::optional rtx_ssrc; uint32_t ssrc = ParseRtpSsrc(packet); - if (unknown_ssrc_packet_buffer_) { - unknown_ssrc_packet_buffer_->AddPacket(ssrc, packet_time_us, packet); - return; - } if (discard_unknown_ssrc_packets_) { return; @@ -1818,52 +1809,6 @@ void WebRtcVideoChannel::OnPacketSent(const rtc::SentPacket& sent_packet) { call_->OnSentPacket(sent_packet); } -void WebRtcVideoChannel::BackfillBufferedPackets( - rtc::ArrayView ssrcs) { - RTC_DCHECK_RUN_ON(&thread_checker_); - if (!unknown_ssrc_packet_buffer_) { - return; - } - - int delivery_ok_cnt = 0; - int delivery_unknown_ssrc_cnt = 0; - int delivery_packet_error_cnt = 0; - webrtc::PacketReceiver* receiver = this->call_->Receiver(); - unknown_ssrc_packet_buffer_->BackfillPackets( - ssrcs, [&](uint32_t /*ssrc*/, int64_t packet_time_us, - rtc::CopyOnWriteBuffer packet) { - switch (receiver->DeliverPacket(webrtc::MediaType::VIDEO, packet, - packet_time_us)) { - case webrtc::PacketReceiver::DELIVERY_OK: - delivery_ok_cnt++; - break; - case webrtc::PacketReceiver::DELIVERY_UNKNOWN_SSRC: - delivery_unknown_ssrc_cnt++; - break; - case webrtc::PacketReceiver::DELIVERY_PACKET_ERROR: - delivery_packet_error_cnt++; - break; - } - }); - rtc::StringBuilder out; - out << "[ "; - for (uint32_t ssrc : ssrcs) { - out << std::to_string(ssrc) << " "; - } - out << "]"; - auto level = rtc::LS_INFO; - if (delivery_unknown_ssrc_cnt > 0 || delivery_packet_error_cnt > 0) { - level = rtc::LS_ERROR; - } - int total = - delivery_ok_cnt + delivery_unknown_ssrc_cnt + delivery_packet_error_cnt; - RTC_LOG_V(level) << "Backfilled " << total - << " packets for ssrcs: " << out.Release() - << " ok: " << delivery_ok_cnt - << " error: " << delivery_packet_error_cnt - << " unknown: " << delivery_unknown_ssrc_cnt; -} - void WebRtcVideoChannel::OnReadyToSend(bool ready) { RTC_DCHECK_RUN_ON(&network_thread_checker_); RTC_LOG(LS_VERBOSE) << "OnReadyToSend: " << (ready ? "Ready." : "Not ready."); @@ -2870,15 +2815,13 @@ void WebRtcVideoChannel::WebRtcVideoSendStream::GenerateKeyFrame( } WebRtcVideoChannel::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream( - WebRtcVideoChannel* channel, webrtc::Call* call, const StreamParams& sp, webrtc::VideoReceiveStreamInterface::Config config, bool default_stream, const std::vector& recv_codecs, const webrtc::FlexfecReceiveStream::Config& flexfec_config) - : channel_(channel), - call_(call), + : call_(call), stream_params_(sp), stream_(NULL), default_stream_(default_stream), @@ -3191,9 +3134,6 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::CreateReceiveStream() { void WebRtcVideoChannel::WebRtcVideoReceiveStream::StartReceiveStream() { stream_->Start(); - if (IsEnabled(call_->trials(), "WebRTC-Video-BufferPacketsWithUnknownSsrc")) { - channel_->BackfillBufferedPackets(stream_params_.ssrcs); - } } void WebRtcVideoChannel::WebRtcVideoReceiveStream::OnFrame( diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index 03732330e5..832d808d63 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -32,7 +32,6 @@ #include "call/video_receive_stream.h" #include "call/video_send_stream.h" #include "media/base/media_engine.h" -#include "media/engine/unhandled_packets_buffer.h" #include "rtc_base/network_route.h" #include "rtc_base/synchronization/mutex.h" #include "rtc_base/system/no_unique_address.h" @@ -235,10 +234,6 @@ class WebRtcVideoChannel : public VideoMediaChannel, std::vector GetSources(uint32_t ssrc) const override; - // Take the buffered packets for `ssrcs` and feed them into DeliverPacket. - // This method does nothing unless unknown_ssrc_packet_buffer_ is configured. - void BackfillBufferedPackets(rtc::ArrayView ssrcs); - // Implements webrtc::EncoderSwitchRequestCallback. void RequestEncoderFallback() override; void RequestEncoderSwitch(const webrtc::SdpVideoFormat& format, @@ -469,7 +464,6 @@ class WebRtcVideoChannel : public VideoMediaChannel, : public rtc::VideoSinkInterface { public: WebRtcVideoReceiveStream( - WebRtcVideoChannel* channel, webrtc::Call* call, const StreamParams& sp, webrtc::VideoReceiveStreamInterface::Config config, @@ -537,7 +531,6 @@ class WebRtcVideoChannel : public VideoMediaChannel, // were applied. bool ReconfigureCodecs(const std::vector& recv_codecs); - WebRtcVideoChannel* const channel_; webrtc::Call* const call_; const StreamParams stream_params_; @@ -681,10 +674,6 @@ class WebRtcVideoChannel : public VideoMediaChannel, rtc::scoped_refptr unsignaled_frame_transformer_ RTC_GUARDED_BY(thread_checker_); - // Buffer for unhandled packets. - std::unique_ptr unknown_ssrc_packet_buffer_ - RTC_GUARDED_BY(thread_checker_); - // TODO(bugs.webrtc.org/11341): Remove this and relevant PC API. Presence // of multiple negotiated codecs allows generic encoder fallback on failures. // Presence of EncoderSelector allows switching to specific encoders.