From 438b5b4ca5bc9d90101d0f058fc3dcf5ecc60885 Mon Sep 17 00:00:00 2001 From: Per K Date: Mon, 23 Jan 2023 14:46:03 +0100 Subject: [PATCH] WebRtcVideoChannel creates default stream with dummy SSRC on received RTX packet. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/main@{#39174} --- media/engine/webrtc_video_engine.cc | 196 +++++++++---------- media/engine/webrtc_video_engine.h | 36 +--- media/engine/webrtc_video_engine_unittest.cc | 103 +++++++++- 3 files changed, 197 insertions(+), 138 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index dfd8d66785..c6ef6037f7 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -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 @@ WebRtcVideoChannel::WebRtcVideoSendStream::ConfigureVideoEncoderSettings( return nullptr; } -DefaultUnsignalledSsrcHandler::DefaultUnsignalledSsrcHandler() - : default_sink_(nullptr) {} - -UnsignalledSsrcHandler::Action DefaultUnsignalledSsrcHandler::OnUnsignalledSsrc( - WebRtcVideoChannel* channel, - uint32_t ssrc, - absl::optional rtx_ssrc) { - absl::optional 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* -DefaultUnsignalledSsrcHandler::GetDefaultSink() const { - return default_sink_; -} - -void DefaultUnsignalledSsrcHandler::SetDefaultSink( - WebRtcVideoChannel* channel, - rtc::VideoSinkInterface* sink) { - default_sink_ = sink; - absl::optional default_recv_ssrc = channel->GetUnsignaledSsrc(); - if (default_recv_ssrc) { - channel->SetSink(*default_recv_ssrc, default_sink_); - } -} - WebRtcVideoEngine::WebRtcVideoEngine( std::unique_ptr video_encoder_factory, std::unique_ptr video_decoder_factory, @@ -724,7 +674,7 @@ WebRtcVideoChannel::WebRtcVideoChannel( : 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 @@ webrtc::RtpParameters WebRtcVideoChannel::GetDefaultRtpReceiveParameters() 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 @@ void WebRtcVideoChannel::SetDefaultSink( rtc::VideoSinkInterface* 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 @@ bool WebRtcVideoChannel::MaybeCreateDefaultReceiveStream( return false; } - absl::optional 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 @@ bool WebRtcVideoChannel::MaybeCreateDefaultReceiveStream( 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 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 rtx_ssrc) { + RTC_DCHECK_RUN_ON(&thread_checker_); + + absl::optional 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 43d53f9c87..0242bdd9f6 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -60,36 +60,6 @@ std::map MergeInfoAboutOutboundRtpSubstreamsForTesting( const std::map& substreams); -class UnsignalledSsrcHandler { - public: - enum Action { - kDropPacket, - kDeliverPacket, - }; - virtual Action OnUnsignalledSsrc(WebRtcVideoChannel* channel, - uint32_t ssrc, - absl::optional 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 rtx_ssrc) override; - - rtc::VideoSinkInterface* GetDefaultSink() const; - void SetDefaultSink(WebRtcVideoChannel* channel, - rtc::VideoSinkInterface* sink); - - virtual ~DefaultUnsignalledSsrcHandler() = default; - - private: - rtc::VideoSinkInterface* default_sink_; -}; - // WebRtcVideoEngine is used for the new native WebRTC Video API (webrtc:1667). class WebRtcVideoEngine : public VideoEngineInterface { public: @@ -321,6 +291,8 @@ class WebRtcVideoChannel : public VideoMediaChannel, bool MaybeCreateDefaultReceiveStream( const webrtc::RtpPacketReceived& parsed_packet) RTC_EXCLUSIVE_LOCKS_REQUIRED(thread_checker_); + void ReCreateDefaulReceiveStream(uint32_t ssrc, + absl::optional rtx_ssrc); void ConfigureReceiverRtp( webrtc::VideoReceiveStreamInterface::Config* config, webrtc::FlexfecReceiveStream::Config* flexfec_config, @@ -600,9 +572,7 @@ class WebRtcVideoChannel : public VideoMediaChannel, 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* 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 1e8f11e6ff..713cfb08c7 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::Return; 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 @@ class MockVideoSource : public rtc::VideoSourceInterface { (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 GetStreamResolutions( const std::vector& streams) { std::vector res; @@ -832,6 +855,51 @@ 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->cdata(), packet->size())); + }), + Return(true))); + std::unique_ptr 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(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 @@ TEST_F(WebRtcVideoChannelTest, Vp9PacketCreatesUnsignalledStream) { 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 @@ TEST_F(WebRtcVideoChannelTest, << "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.