WebRtcVideoChannel creates default stream with dummy SSRC on received RTX packet.
This ensure transport feedback is sent for RTX packets that are received before media payload packets.
Bug: webrtc:14795, webrtc:14817
Change-Id: I6a2579b87c8863e003decb2b2559ef51a852cadb
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/291119
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39174}
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc
index dfd8d66..c6ef603 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -13,6 +13,7 @@
#include <stdio.h>
#include <algorithm>
+#include <cstdint>
#include <set>
#include <string>
#include <utility>
@@ -20,6 +21,7 @@
#include "absl/algorithm/container.h"
#include "absl/functional/bind_front.h"
#include "absl/strings/match.h"
+#include "absl/types/optional.h"
#include "api/media_stream_interface.h"
#include "api/video/video_codec_constants.h"
#include "api/video/video_codec_type.h"
@@ -583,58 +585,6 @@
return nullptr;
}
-DefaultUnsignalledSsrcHandler::DefaultUnsignalledSsrcHandler()
- : default_sink_(nullptr) {}
-
-UnsignalledSsrcHandler::Action DefaultUnsignalledSsrcHandler::OnUnsignalledSsrc(
- WebRtcVideoChannel* channel,
- uint32_t ssrc,
- absl::optional<uint32_t> rtx_ssrc) {
- absl::optional<uint32_t> default_recv_ssrc = channel->GetUnsignaledSsrc();
-
- if (default_recv_ssrc) {
- RTC_LOG(LS_INFO) << "Destroying old default receive stream for SSRC="
- << ssrc << ".";
- channel->RemoveRecvStream(*default_recv_ssrc);
- }
-
- StreamParams sp = channel->unsignaled_stream_params();
- sp.ssrcs.push_back(ssrc);
- if (rtx_ssrc) {
- sp.AddFidSsrc(ssrc, *rtx_ssrc);
- }
- RTC_LOG(LS_INFO) << "Creating default receive stream for SSRC=" << ssrc
- << ".";
- if (!channel->AddRecvStream(sp, /*default_stream=*/true)) {
- RTC_LOG(LS_WARNING) << "Could not create default receive stream.";
- }
-
- // SSRC 0 returns default_recv_base_minimum_delay_ms.
- const int unsignaled_ssrc = 0;
- int default_recv_base_minimum_delay_ms =
- channel->GetBaseMinimumPlayoutDelayMs(unsignaled_ssrc).value_or(0);
- // Set base minimum delay if it was set before for the default receive stream.
- channel->SetBaseMinimumPlayoutDelayMs(ssrc,
- default_recv_base_minimum_delay_ms);
- channel->SetSink(ssrc, default_sink_);
- return kDeliverPacket;
-}
-
-rtc::VideoSinkInterface<webrtc::VideoFrame>*
-DefaultUnsignalledSsrcHandler::GetDefaultSink() const {
- return default_sink_;
-}
-
-void DefaultUnsignalledSsrcHandler::SetDefaultSink(
- WebRtcVideoChannel* channel,
- rtc::VideoSinkInterface<webrtc::VideoFrame>* sink) {
- default_sink_ = sink;
- absl::optional<uint32_t> default_recv_ssrc = channel->GetUnsignaledSsrc();
- if (default_recv_ssrc) {
- channel->SetSink(*default_recv_ssrc, default_sink_);
- }
-}
-
WebRtcVideoEngine::WebRtcVideoEngine(
std::unique_ptr<webrtc::VideoEncoderFactory> video_encoder_factory,
std::unique_ptr<webrtc::VideoDecoderFactory> video_decoder_factory,
@@ -724,7 +674,7 @@
: VideoMediaChannel(call->network_thread(), config.enable_dscp),
worker_thread_(call->worker_thread()),
call_(call),
- unsignalled_ssrc_handler_(&default_unsignalled_ssrc_handler_),
+ default_sink_(nullptr),
video_config_(config.video),
encoder_factory_(encoder_factory),
decoder_factory_(decoder_factory),
@@ -1151,7 +1101,7 @@
const {
RTC_DCHECK_RUN_ON(&thread_checker_);
webrtc::RtpParameters rtp_params;
- if (!default_unsignalled_ssrc_handler_.GetDefaultSink()) {
+ if (!default_sink_) {
// Getting parameters on a default, unsignaled video receive stream but
// because we've not configured to receive such a stream, `encodings` is
// empty.
@@ -1648,7 +1598,7 @@
rtc::VideoSinkInterface<webrtc::VideoFrame>* sink) {
RTC_DCHECK_RUN_ON(&thread_checker_);
RTC_LOG(LS_INFO) << "SetDefaultSink: " << (sink ? "(ptr)" : "nullptr");
- default_unsignalled_ssrc_handler_.SetDefaultSink(this, sink);
+ default_sink_ = sink;
}
bool WebRtcVideoChannel::GetSendStats(VideoMediaSendInfo* info) {
@@ -1809,35 +1759,6 @@
return false;
}
- absl::optional<uint32_t> rtx_ssrc;
- uint32_t ssrc = packet.Ssrc();
- // See if this payload_type is registered as one that usually gets its
- // own SSRC (RTX) or at least is safe to drop either way (FEC). If it
- // is, and it wasn't handled above by DeliverPacket, that means we don't
- // know what stream it associates with, and we shouldn't ever create an
- // implicit channel for these.
- for (auto& codec : recv_codecs_) {
- if (packet.PayloadType() == codec.ulpfec.red_rtx_payload_type ||
- packet.PayloadType() == codec.ulpfec.ulpfec_payload_type) {
- return false;
- }
- if (packet.PayloadType() == codec.rtx_payload_type) {
- // As we don't support receiving simulcast there can only be one RTX
- // stream, which will be associated with unsignaled media stream.
- // It is not possible to update the ssrcs of a receive stream, so we
- // recreate it insead if found.
- auto default_ssrc = GetUnsignaledSsrc();
- if (!default_ssrc) {
- return false;
- }
- rtx_ssrc = ssrc;
- ssrc = *default_ssrc;
- // Allow recreating the receive stream even if the RTX packet is
- // received just after the media packet.
- last_unsignalled_ssrc_creation_time_ms_.reset();
- break;
- }
- }
if (packet.PayloadType() == recv_flexfec_payload_type_) {
return false;
}
@@ -1849,33 +1770,102 @@
if (demuxer_criteria_id_ != demuxer_criteria_completed_id_) {
return false;
}
- // Ignore unknown ssrcs if we recently created an unsignalled receive
- // stream since this shouldn't happen frequently. Getting into a state
- // of creating decoders on every packet eats up processing time (e.g.
- // https://crbug.com/1069603) and this cooldown prevents that.
- if (last_unsignalled_ssrc_creation_time_ms_.has_value()) {
- int64_t now_ms = rtc::TimeMillis();
- if (now_ms - last_unsignalled_ssrc_creation_time_ms_.value() <
- kUnsignaledSsrcCooldownMs) {
- // We've already created an unsignalled ssrc stream within the last
- // 0.5 s, ignore with a warning.
- RTC_LOG(LS_WARNING)
- << "Another unsignalled ssrc packet arrived shortly after the "
- << "creation of an unsignalled ssrc stream. Dropping packet.";
+
+ // See if this payload_type is registered as one that usually gets its
+ // own SSRC (RTX) or at least is safe to drop either way (FEC). If it
+ // is, and it wasn't handled above by DeliverPacket, that means we don't
+ // know what stream it associates with, and we shouldn't ever create an
+ // implicit channel for these.
+ bool is_rtx_payload = false;
+ for (auto& codec : recv_codecs_) {
+ if (packet.PayloadType() == codec.ulpfec.red_rtx_payload_type ||
+ packet.PayloadType() == codec.ulpfec.ulpfec_payload_type) {
return false;
}
- }
- // Let the unsignalled ssrc handler decide whether to drop or deliver.
- switch (unsignalled_ssrc_handler_->OnUnsignalledSsrc(this, ssrc, rtx_ssrc)) {
- case UnsignalledSsrcHandler::kDropPacket:
- return false;
- case UnsignalledSsrcHandler::kDeliverPacket:
+
+ if (packet.PayloadType() == codec.rtx_payload_type) {
+ is_rtx_payload = true;
break;
+ }
}
+
+ if (is_rtx_payload) {
+ // As we don't support receiving simulcast there can only be one RTX
+ // stream, which will be associated with unsignaled media stream.
+ absl::optional<uint32_t> current_default_ssrc = GetUnsignaledSsrc();
+ if (current_default_ssrc) {
+ // TODO(bug.webrtc.org/14817): Consider associating the existing default
+ // stream with this RTX stream instead of recreating.
+ ReCreateDefaulReceiveStream(/*ssrc =*/*current_default_ssrc,
+ packet.Ssrc());
+ } else {
+ // Received unsignaled RTX packet before a media packet. Create a default
+ // stream with a "random" SSRC and the RTX SSRC from the packet. The
+ // stream will be recreated on the first media packet, unless we are
+ // extremely lucky and used the right media SSRC.
+ ReCreateDefaulReceiveStream(/*ssrc =*/14795, /*rtx_ssrc=*/packet.Ssrc());
+ }
+ return true;
+ } else {
+ // Ignore unknown ssrcs if we recently created an unsignalled receive
+ // stream since this shouldn't happen frequently. Getting into a state
+ // of creating decoders on every packet eats up processing time (e.g.
+ // https://crbug.com/1069603) and this cooldown prevents that.
+ if (last_unsignalled_ssrc_creation_time_ms_.has_value()) {
+ int64_t now_ms = rtc::TimeMillis();
+ if (now_ms - last_unsignalled_ssrc_creation_time_ms_.value() <
+ kUnsignaledSsrcCooldownMs) {
+ // We've already created an unsignalled ssrc stream within the last
+ // 0.5 s, ignore with a warning.
+ RTC_LOG(LS_WARNING)
+ << "Another unsignalled ssrc packet arrived shortly after the "
+ << "creation of an unsignalled ssrc stream. Dropping packet.";
+ return false;
+ }
+ }
+ }
+
+ // TODO(bug.webrtc.org/14817): Consider creating a default stream with a fake
+ // RTX ssrc that can be updated when the real SSRC is known if rtx has been
+ // negotiated.
+ ReCreateDefaulReceiveStream(packet.Ssrc(), absl::nullopt);
last_unsignalled_ssrc_creation_time_ms_ = rtc::TimeMillis();
return true;
}
+void WebRtcVideoChannel::ReCreateDefaulReceiveStream(
+ uint32_t ssrc,
+ absl::optional<uint32_t> rtx_ssrc) {
+ RTC_DCHECK_RUN_ON(&thread_checker_);
+
+ absl::optional<uint32_t> default_recv_ssrc = GetUnsignaledSsrc();
+ if (default_recv_ssrc) {
+ RTC_LOG(LS_INFO) << "Destroying old default receive stream for SSRC="
+ << ssrc << ".";
+ RemoveRecvStream(*default_recv_ssrc);
+ }
+
+ StreamParams sp = unsignaled_stream_params();
+ sp.ssrcs.push_back(ssrc);
+ if (rtx_ssrc) {
+ sp.AddFidSsrc(ssrc, *rtx_ssrc);
+ }
+ RTC_LOG(LS_INFO) << "Creating default receive stream for SSRC=" << ssrc
+ << ".";
+ if (!AddRecvStream(sp, /*default_stream=*/true)) {
+ RTC_LOG(LS_WARNING) << "Could not create default receive stream.";
+ }
+
+ // SSRC 0 returns default_recv_base_minimum_delay_ms.
+ const int unsignaled_ssrc = 0;
+ int default_recv_base_minimum_delay_ms =
+ GetBaseMinimumPlayoutDelayMs(unsignaled_ssrc).value_or(0);
+ // Set base minimum delay if it was set before for the default receive
+ // stream.
+ SetBaseMinimumPlayoutDelayMs(ssrc, default_recv_base_minimum_delay_ms);
+ SetSink(ssrc, default_sink_);
+}
+
void WebRtcVideoChannel::OnPacketSent(const rtc::SentPacket& sent_packet) {
RTC_DCHECK_RUN_ON(&network_thread_checker_);
// TODO(tommi): We shouldn't need to go through call_ to deliver this
diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h
index 43d53f9..0242bdd 100644
--- a/media/engine/webrtc_video_engine.h
+++ b/media/engine/webrtc_video_engine.h
@@ -60,36 +60,6 @@
MergeInfoAboutOutboundRtpSubstreamsForTesting(
const std::map<uint32_t, webrtc::VideoSendStream::StreamStats>& substreams);
-class UnsignalledSsrcHandler {
- public:
- enum Action {
- kDropPacket,
- kDeliverPacket,
- };
- virtual Action OnUnsignalledSsrc(WebRtcVideoChannel* channel,
- uint32_t ssrc,
- absl::optional<uint32_t> rtx_ssrc) = 0;
- virtual ~UnsignalledSsrcHandler() = default;
-};
-
-// TODO(pbos): Remove, use external handlers only.
-class DefaultUnsignalledSsrcHandler : public UnsignalledSsrcHandler {
- public:
- DefaultUnsignalledSsrcHandler();
- Action OnUnsignalledSsrc(WebRtcVideoChannel* channel,
- uint32_t ssrc,
- absl::optional<uint32_t> rtx_ssrc) override;
-
- rtc::VideoSinkInterface<webrtc::VideoFrame>* GetDefaultSink() const;
- void SetDefaultSink(WebRtcVideoChannel* channel,
- rtc::VideoSinkInterface<webrtc::VideoFrame>* sink);
-
- virtual ~DefaultUnsignalledSsrcHandler() = default;
-
- private:
- rtc::VideoSinkInterface<webrtc::VideoFrame>* default_sink_;
-};
-
// WebRtcVideoEngine is used for the new native WebRTC Video API (webrtc:1667).
class WebRtcVideoEngine : public VideoEngineInterface {
public:
@@ -321,6 +291,8 @@
bool MaybeCreateDefaultReceiveStream(
const webrtc::RtpPacketReceived& parsed_packet)
RTC_EXCLUSIVE_LOCKS_REQUIRED(thread_checker_);
+ void ReCreateDefaulReceiveStream(uint32_t ssrc,
+ absl::optional<uint32_t> rtx_ssrc);
void ConfigureReceiverRtp(
webrtc::VideoReceiveStreamInterface::Config* config,
webrtc::FlexfecReceiveStream::Config* flexfec_config,
@@ -600,9 +572,7 @@
bool sending_ RTC_GUARDED_BY(thread_checker_);
webrtc::Call* const call_;
- DefaultUnsignalledSsrcHandler default_unsignalled_ssrc_handler_
- RTC_GUARDED_BY(thread_checker_);
- UnsignalledSsrcHandler* const unsignalled_ssrc_handler_
+ rtc::VideoSinkInterface<webrtc::VideoFrame>* default_sink_
RTC_GUARDED_BY(thread_checker_);
// Delay for unsignaled streams, which may be set before the stream exists.
diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc
index 1e8f11e..713cfb0 100644
--- a/media/engine/webrtc_video_engine_unittest.cc
+++ b/media/engine/webrtc_video_engine_unittest.cc
@@ -53,6 +53,8 @@
#include "media/engine/fake_webrtc_call.h"
#include "media/engine/fake_webrtc_video_engine.h"
#include "media/engine/webrtc_voice_engine.h"
+#include "modules/rtp_rtcp/include/rtp_header_extension_map.h"
+#include "modules/rtp_rtcp/source/rtcp_packet.h"
#include "modules/rtp_rtcp/source/rtp_packet.h"
#include "modules/rtp_rtcp/source/rtp_packet_received.h"
#include "modules/video_coding/svc/scalability_mode_util.h"
@@ -66,6 +68,7 @@
#include "test/fake_decoder.h"
#include "test/frame_forwarder.h"
#include "test/gmock.h"
+#include "test/rtcp_packet_parser.h"
#include "test/scoped_key_value_config.h"
#include "test/time_controller/simulated_time_controller.h"
#include "video/config/simulcast.h"
@@ -85,12 +88,14 @@
using ::testing::SizeIs;
using ::testing::StrNe;
using ::testing::Values;
+using ::testing::WithArg;
using ::webrtc::BitrateConstraints;
using ::webrtc::kDefaultScalabilityModeStr;
using ::webrtc::RtpExtension;
using ::webrtc::RtpPacket;
using ::webrtc::RtpPacketReceived;
using ::webrtc::ScalabilityMode;
+using ::webrtc::test::RtcpPacketParser;
namespace {
static const int kDefaultQpMax = 56;
@@ -246,6 +251,24 @@
(override));
};
+class MockNetworkInterface : public cricket::MediaChannelNetworkInterface {
+ public:
+ MOCK_METHOD(bool,
+ SendPacket,
+ (rtc::CopyOnWriteBuffer * packet,
+ const rtc::PacketOptions& options),
+ (override));
+ MOCK_METHOD(bool,
+ SendRtcp,
+ (rtc::CopyOnWriteBuffer * packet,
+ const rtc::PacketOptions& options),
+ (override));
+ MOCK_METHOD(int,
+ SetOption,
+ (SocketType type, rtc::Socket::Option opt, int option),
+ (override));
+};
+
std::vector<webrtc::Resolution> GetStreamResolutions(
const std::vector<webrtc::VideoStream>& streams) {
std::vector<webrtc::Resolution> res;
@@ -832,6 +855,51 @@
}
}
+TEST_F(WebRtcVideoEngineTest, SendsFeedbackAfterUnsignaledRtxPacket) {
+ // Setup a channel with VP8, RTX and transport sequence number header
+ // extension. Receive stream is not explicitly configured.
+ AddSupportedVideoCodecType("VP8");
+ std::vector<VideoCodec> supported_codecs =
+ engine_.recv_codecs(/*include_rtx=*/true);
+ ASSERT_EQ(supported_codecs[1].name, "rtx");
+ int rtx_payload_type = supported_codecs[1].id;
+ MockNetworkInterface network;
+ RtcpPacketParser rtcp_parser;
+ ON_CALL(network, SendRtcp)
+ .WillByDefault(testing::DoAll(
+ WithArg<0>([&](rtc::CopyOnWriteBuffer* packet) {
+ ASSERT_TRUE(rtcp_parser.Parse(packet->cdata(), packet->size()));
+ }),
+ Return(true)));
+ std::unique_ptr<VideoMediaChannel> channel(engine_.CreateMediaChannel(
+ call_.get(), GetMediaConfig(), VideoOptions(), webrtc::CryptoOptions(),
+ video_bitrate_allocator_factory_.get()));
+ cricket::VideoRecvParameters parameters;
+ parameters.codecs = supported_codecs;
+ const int kTransportSeqExtensionId = 1;
+ parameters.extensions.push_back(RtpExtension(
+ RtpExtension::kTransportSequenceNumberUri, kTransportSeqExtensionId));
+ ASSERT_TRUE(channel->SetRecvParameters(parameters));
+ channel->SetInterface(&network);
+ channel->AsVideoReceiveChannel()->OnReadyToSend(true);
+
+ // Inject a RTX packet.
+ webrtc::RtpHeaderExtensionMap extension_map(parameters.extensions);
+ webrtc::RtpPacketReceived packet(&extension_map);
+ packet.SetMarker(true);
+ packet.SetPayloadType(rtx_payload_type);
+ packet.SetSsrc(999);
+ packet.SetExtension<webrtc::TransportSequenceNumber>(7);
+ uint8_t* buf_ptr = packet.AllocatePayload(11);
+ memset(buf_ptr, 0, 11); // Pass MSAN (don't care about bytes 1-9)
+ channel->AsVideoReceiveChannel()->OnPacketReceived(packet);
+
+ // Expect that feedback is sent after a while.
+ time_controller_.AdvanceTime(webrtc::TimeDelta::Seconds(1));
+ EXPECT_GT(rtcp_parser.transport_feedback()->num_packets(), 0);
+
+ channel->SetInterface(nullptr);
+}
TEST_F(WebRtcVideoEngineTest, UsesSimulcastAdapterForVp8Factories) {
AddSupportedVideoCodecType("VP8");
@@ -7178,12 +7246,12 @@
true /* expect_created_receive_stream */);
}
-TEST_F(WebRtcVideoChannelTest, RtxPacketDoesntCreateUnsignalledStream) {
+TEST_F(WebRtcVideoChannelTest, RtxPacketCreateUnsignalledStream) {
AssignDefaultAptRtxTypes();
const cricket::VideoCodec vp8 = GetEngineCodec("VP8");
const int rtx_vp8_payload_type = default_apt_rtx_types_[vp8.id];
TestReceiveUnsignaledSsrcPacket(rtx_vp8_payload_type,
- false /* expect_created_receive_stream */);
+ true /* expect_created_receive_stream */);
}
TEST_F(WebRtcVideoChannelTest, UlpfecPacketDoesntCreateUnsignalledStream) {
@@ -7237,6 +7305,37 @@
<< "Receive stream should have correct rtx ssrc";
}
+TEST_F(WebRtcVideoChannelTest,
+ MediaPacketAfterRtxImmediatelyRecreatesUnsignalledStream) {
+ AssignDefaultAptRtxTypes();
+ const cricket::VideoCodec vp8 = GetEngineCodec("VP8");
+ const int payload_type = vp8.id;
+ const int rtx_vp8_payload_type = default_apt_rtx_types_[vp8.id];
+ const uint32_t ssrc = kIncomingUnsignalledSsrc;
+ const uint32_t rtx_ssrc = ssrc + 1;
+
+ // Send rtx packet.
+ RtpPacketReceived rtx_packet;
+ rtx_packet.SetPayloadType(rtx_vp8_payload_type);
+ rtx_packet.SetSsrc(rtx_ssrc);
+ receive_channel_->OnPacketReceived(rtx_packet);
+ rtc::Thread::Current()->ProcessMessages(0);
+ EXPECT_EQ(1u, fake_call_->GetVideoReceiveStreams().size());
+
+ // Send media packet.
+ RtpPacketReceived packet;
+ packet.SetPayloadType(payload_type);
+ packet.SetSsrc(ssrc);
+ ReceivePacketAndAdvanceTime(packet);
+ EXPECT_EQ(1u, fake_call_->GetVideoReceiveStreams().size());
+
+ // Check receive stream has been recreated with correct ssrcs.
+ auto recv_stream = fake_call_->GetVideoReceiveStreams().front();
+ auto& config = recv_stream->GetConfig();
+ EXPECT_EQ(config.rtp.remote_ssrc, ssrc)
+ << "Receive stream should have correct media ssrc";
+}
+
// Test that receiving any unsignalled SSRC works even if it changes.
// The first unsignalled SSRC received will create a default receive stream.
// Any different unsignalled SSRC received will replace the default.
