From d440358ccaeb9aa5dc4a50c0c37638416a15cac5 Mon Sep 17 00:00:00 2001 From: Per K Date: Thu, 22 Feb 2024 13:45:16 +0100 Subject: [PATCH] Dont create RTX receive stream before media SSRC is known The feauture was added in https://webrtc-review.googlesource.com/c/src/+/291119 in order to ensure RTX packet is part of BWE even before the RTP stream is known. However, it cause an issue if media is signaled with an SSRC that has this RTX SSRC. Since BWE is now notified about received packets before demuxing to the correct receive stream, it is not necessary to demux RTX packets before the media SSRC is known. Note that WebRTC require at least one negotiated SSRC/MID before RTCP feedback can be sent. Ie, for BWE to work, at least one media SSRC must be known after this cl. It can either be unsignaled or signaled. BWE tested with BweRampupWithInitialProbeTest. Bug: webrtc:14795, webrtc:14817, b/320258158 Change-Id: Icf2c67bedc352720bf846b9ee38d509346af36f2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/340141 Reviewed-by: Harald Alvestrand Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#41785} --- call/packet_receiver.h | 2 +- media/engine/webrtc_video_engine.cc | 15 ++--- media/engine/webrtc_video_engine_unittest.cc | 66 ++------------------ 3 files changed, 13 insertions(+), 70 deletions(-) diff --git a/call/packet_receiver.h b/call/packet_receiver.h index cdcf7bfc73..6d223efdfa 100644 --- a/call/packet_receiver.h +++ b/call/packet_receiver.h @@ -23,7 +23,7 @@ class PacketReceiver { // Demux RTCP packets. Must be called on the worker thread. virtual void DeliverRtcpPacket(rtc::CopyOnWriteBuffer packet) = 0; - // Invoked once when a packet packet is received that can not be demuxed. + // Invoked once when a packet is received that can not be demuxed. // If the method returns true, a new attempt is made to demux the packet. using OnUndemuxablePacketHandler = absl::AnyInvocable; diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index a313407f2d..a36b802334 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -3117,15 +3117,12 @@ bool WebRtcVideoReceiveChannel::MaybeCreateDefaultReceiveStream( absl::optional current_default_ssrc = GetUnsignaledSsrc(); if (current_default_ssrc) { FindReceiveStream(*current_default_ssrc)->UpdateRtxSsrc(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. - ReCreateDefaultReceiveStream(/*ssrc =*/14795, /*rtx_ssrc=*/packet.Ssrc()); + return true; } - return true; - } else { + // Default media SSRC not known yet. Drop the packet. + // BWE has already been notified of this received packet. + 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. @@ -3142,7 +3139,7 @@ bool WebRtcVideoReceiveChannel::MaybeCreateDefaultReceiveStream( return false; } } - } + // RTX SSRC not yet known. ReCreateDefaultReceiveStream(packet.Ssrc(), absl::nullopt); last_unsignalled_ssrc_creation_time_ms_ = rtc::TimeMillis(); diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 4493c9c616..4bb795c1c1 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -922,59 +922,6 @@ void WebRtcVideoEngineTest::ExpectRtpCapabilitySupport(const char* uri, } } -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 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)); - }), - Return(true))); - std::unique_ptr send_channel = - engine_.CreateSendChannel(call_.get(), GetMediaConfig(), VideoOptions(), - webrtc::CryptoOptions(), - video_bitrate_allocator_factory_.get()); - std::unique_ptr receive_channel = - engine_.CreateReceiveChannel(call_.get(), GetMediaConfig(), - VideoOptions(), webrtc::CryptoOptions()); - cricket::VideoReceiverParameters parameters; - parameters.codecs = supported_codecs; - const int kTransportSeqExtensionId = 1; - parameters.extensions.push_back(RtpExtension( - RtpExtension::kTransportSequenceNumberUri, kTransportSeqExtensionId)); - ASSERT_TRUE(receive_channel->SetReceiverParameters(parameters)); - send_channel->SetInterface(&network); - receive_channel->SetInterface(&network); - send_channel->OnReadyToSend(true); - receive_channel->SetReceive(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(7); - uint8_t* buf_ptr = packet.AllocatePayload(11); - memset(buf_ptr, 0, 11); // Pass MSAN (don't care about bytes 1-9) - receive_channel->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); - - send_channel->SetInterface(nullptr); - receive_channel->SetInterface(nullptr); -} - TEST_F(WebRtcVideoEngineTest, ReceiveBufferSizeViaFieldTrial) { webrtc::test::ScopedKeyValueConfig override_field_trials( field_trials_, "WebRTC-ReceiveBufferSize/size_bytes:10000/"); @@ -7475,12 +7422,12 @@ TEST_F(WebRtcVideoChannelTest, Vp9PacketCreatesUnsignalledStream) { true /* expect_created_receive_stream */); } -TEST_F(WebRtcVideoChannelTest, RtxPacketCreateUnsignalledStream) { +TEST_F(WebRtcVideoChannelTest, RtxPacketDoesntCreateUnsignalledStream) { AssignDefaultAptRtxTypes(); const cricket::VideoCodec vp8 = GetEngineCodec("VP8"); const int rtx_vp8_payload_type = default_apt_rtx_types_[vp8.id]; TestReceiveUnsignaledSsrcPacket(rtx_vp8_payload_type, - true /* expect_created_receive_stream */); + false /* expect_created_receive_stream */); } TEST_F(WebRtcVideoChannelTest, UlpfecPacketDoesntCreateUnsignalledStream) { @@ -7534,8 +7481,7 @@ TEST_F(WebRtcVideoChannelTest, RtxAfterMediaPacketUpdatesUnsignalledRtxSsrc) { EXPECT_EQ(fake_call_->GetDeliveredPacketsForSsrc(rtx_ssrc), 1u); } -TEST_F(WebRtcVideoChannelTest, - MediaPacketAfterRtxImmediatelyRecreatesUnsignalledStream) { +TEST_F(WebRtcVideoChannelTest, UnsignaledStreamCreatedAfterMediaPacket) { AssignDefaultAptRtxTypes(); const cricket::VideoCodec vp8 = GetEngineCodec("VP8"); const int payload_type = vp8.id; @@ -7543,15 +7489,15 @@ TEST_F(WebRtcVideoChannelTest, const uint32_t ssrc = kIncomingUnsignalledSsrc; const uint32_t rtx_ssrc = ssrc + 1; - // Send rtx packet. + // Receive rtx packet. RtpPacketReceived rtx_packet; rtx_packet.SetPayloadType(rtx_vp8_payload_type); rtx_packet.SetSsrc(rtx_ssrc); receive_channel_->OnPacketReceived(rtx_packet); time_controller_.AdvanceTime(TimeDelta::Zero()); - EXPECT_EQ(1u, fake_call_->GetVideoReceiveStreams().size()); + EXPECT_EQ(0u, fake_call_->GetVideoReceiveStreams().size()); - // Send media packet. + // Receive media packet. RtpPacketReceived packet; packet.SetPayloadType(payload_type); packet.SetSsrc(ssrc);