From 11fb472ae40de4e03e5b12aa0bb3f99d7e9c1a69 Mon Sep 17 00:00:00 2001 From: brandtr Date: Tue, 30 May 2017 01:31:37 -0700 Subject: [PATCH] Recreate FlexfecReceiveStream separately from VideoReceiveStream. After this CL, reconfiguring the FlexFEC payload type at the WebRtcVideoChannel2 level will no longer lead to the recreation of the VideoReceiveStream. This means that the jitter buffer will not be destroyed and a smoother video playback is achieved during SDP renegotiation. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2911913002 Cr-Commit-Position: refs/heads/master@{#18318} --- webrtc/media/engine/webrtcvideoengine2.cc | 100 +++++++++++++----- webrtc/media/engine/webrtcvideoengine2.h | 23 +++- .../engine/webrtcvideoengine2_unittest.cc | 47 ++++++++ 3 files changed, 142 insertions(+), 28 deletions(-) diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index abb35843c0..03e97f776b 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -654,6 +654,7 @@ WebRtcVideoChannel2::WebRtcVideoChannel2( rtcp_receiver_report_ssrc_ = kDefaultRtcpReceiverReportSsrc; sending_ = false; recv_codecs_ = MapCodecs(GetSupportedCodecs(external_encoder_factory)); + recv_flexfec_payload_type_ = recv_codecs_.front().flexfec_payload_type; } WebRtcVideoChannel2::~WebRtcVideoChannel2() { @@ -682,12 +683,13 @@ WebRtcVideoChannel2::SelectSendVideoCodec( return rtc::Optional(); } -bool WebRtcVideoChannel2::ReceiveCodecsHaveChanged( +bool WebRtcVideoChannel2::NonFlexfecReceiveCodecsHaveChanged( std::vector before, std::vector after) { if (before.size() != after.size()) { return true; } + // The receive codec order doesn't matter, so we sort the codecs before // comparing. This is necessary because currently the // only way to change the send codec is to munge SDP, which causes @@ -702,7 +704,12 @@ bool WebRtcVideoChannel2::ReceiveCodecsHaveChanged( }; std::sort(before.begin(), before.end(), comparison); std::sort(after.begin(), after.end(), comparison); - return before != after; + + // Changes in FlexFEC payload type are handled separately in + // WebRtcVideoChannel2::GetChangedRecvParameters, so disregard FlexFEC in the + // comparison here. + return !std::equal(before.begin(), before.end(), after.begin(), + VideoCodecSettings::EqualsDisregardingFlexfec); } bool WebRtcVideoChannel2::GetChangedSendParameters( @@ -982,7 +989,7 @@ bool WebRtcVideoChannel2::GetChangedRecvParameters( } } - if (ReceiveCodecsHaveChanged(recv_codecs_, mapped_codecs)) { + if (NonFlexfecReceiveCodecsHaveChanged(recv_codecs_, mapped_codecs)) { changed_params->codec_settings = rtc::Optional>(mapped_codecs); } @@ -995,6 +1002,12 @@ bool WebRtcVideoChannel2::GetChangedRecvParameters( rtc::Optional>(filtered_extensions); } + int flexfec_payload_type = mapped_codecs.front().flexfec_payload_type; + if (flexfec_payload_type != recv_flexfec_payload_type_) { + changed_params->flexfec_payload_type = + rtc::Optional(flexfec_payload_type); + } + return true; } @@ -1005,6 +1018,12 @@ bool WebRtcVideoChannel2::SetRecvParameters(const VideoRecvParameters& params) { if (!GetChangedRecvParameters(params, &changed_params)) { return false; } + if (changed_params.flexfec_payload_type) { + LOG(LS_INFO) << "Changing FlexFEC payload type (recv) from " + << recv_flexfec_payload_type_ << " to " + << *changed_params.flexfec_payload_type; + recv_flexfec_payload_type_ = *changed_params.flexfec_payload_type; + } if (changed_params.rtp_header_extensions) { recv_rtp_extensions_ = *changed_params.rtp_header_extensions; } @@ -1288,6 +1307,7 @@ void WebRtcVideoChannel2::ConfigureReceiverRtp( config->rtp.extensions = recv_rtp_extensions_; // TODO(brandtr): Generalize when we add support for multistream protection. + flexfec_config->payload_type = recv_flexfec_payload_type_; if (IsFlexfecAdvertisedFieldTrialEnabled() && sp.GetFecFrSsrc(ssrc, &flexfec_config->remote_ssrc)) { flexfec_config->protected_media_ssrcs = {ssrc}; @@ -1464,11 +1484,13 @@ void WebRtcVideoChannel2::OnPacketReceived( for (auto& codec : recv_codecs_) { if (payload_type == codec.rtx_payload_type || payload_type == codec.ulpfec.red_rtx_payload_type || - payload_type == codec.ulpfec.ulpfec_payload_type || - payload_type == codec.flexfec_payload_type) { + payload_type == codec.ulpfec.ulpfec_payload_type) { return; } } + if (payload_type == recv_flexfec_payload_type_) { + return; + } switch (unsignalled_ssrc_handler_->OnUnsignalledSsrc(this, ssrc)) { case UnsignalledSsrcHandler::kDropPacket: @@ -2199,7 +2221,9 @@ WebRtcVideoChannel2::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream( config_.renderer = this; std::vector old_decoders; ConfigureCodecs(recv_codecs, &old_decoders); - RecreateWebRtcStream(); + ConfigureFlexfecCodec(flexfec_config.payload_type); + MaybeRecreateWebRtcFlexfecStream(); + RecreateWebRtcVideoStream(); RTC_DCHECK(old_decoders.empty()); } @@ -2301,12 +2325,16 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::ConfigureCodecs( } config_.rtp.ulpfec = recv_codecs.front().ulpfec; - flexfec_config_.payload_type = recv_codecs.front().flexfec_payload_type; config_.rtp.nack.rtp_history_ms = HasNack(recv_codecs.begin()->codec) ? kNackHistoryMs : 0; } +void WebRtcVideoChannel2::WebRtcVideoReceiveStream::ConfigureFlexfecCodec( + int flexfec_payload_type) { + flexfec_config_.payload_type = flexfec_payload_type; +} + void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetLocalSsrc( uint32_t local_ssrc) { // TODO(pbos): Consider turning this sanity check into a RTC_DCHECK. You @@ -2324,7 +2352,8 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetLocalSsrc( LOG(LS_INFO) << "RecreateWebRtcStream (recv) because of SetLocalSsrc; local_ssrc=" << local_ssrc; - RecreateWebRtcStream(); + MaybeRecreateWebRtcFlexfecStream(); + RecreateWebRtcVideoStream(); } void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetFeedbackParameters( @@ -2356,47 +2385,61 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetFeedbackParameters( << "RecreateWebRtcStream (recv) because of SetFeedbackParameters; nack=" << nack_enabled << ", remb=" << remb_enabled << ", transport_cc=" << transport_cc_enabled; - RecreateWebRtcStream(); + MaybeRecreateWebRtcFlexfecStream(); + RecreateWebRtcVideoStream(); } void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRecvParameters( const ChangedRecvParameters& params) { - bool needs_recreation = false; + bool video_needs_recreation = false; + bool flexfec_needs_recreation = false; std::vector old_decoders; if (params.codec_settings) { ConfigureCodecs(*params.codec_settings, &old_decoders); - needs_recreation = true; + video_needs_recreation = true; } if (params.rtp_header_extensions) { config_.rtp.extensions = *params.rtp_header_extensions; flexfec_config_.rtp_header_extensions = *params.rtp_header_extensions; - needs_recreation = true; + video_needs_recreation = true; + flexfec_needs_recreation = true; } - if (needs_recreation) { - LOG(LS_INFO) << "RecreateWebRtcStream (recv) because of SetRecvParameters"; - RecreateWebRtcStream(); + if (params.flexfec_payload_type) { + ConfigureFlexfecCodec(*params.flexfec_payload_type); + flexfec_needs_recreation = true; + } + if (flexfec_needs_recreation) { + LOG(LS_INFO) << "MaybeRecreateWebRtcFlexfecStream (recv) because of " + "SetRecvParameters"; + MaybeRecreateWebRtcFlexfecStream(); + } + if (video_needs_recreation) { + LOG(LS_INFO) + << "RecreateWebRtcVideoStream (recv) because of SetRecvParameters"; + RecreateWebRtcVideoStream(); ClearDecoders(&old_decoders); } } -void WebRtcVideoChannel2::WebRtcVideoReceiveStream::RecreateWebRtcStream() { +void WebRtcVideoChannel2::WebRtcVideoReceiveStream:: + RecreateWebRtcVideoStream() { if (stream_) { call_->DestroyVideoReceiveStream(stream_); stream_ = nullptr; } + webrtc::VideoReceiveStream::Config config = config_.Copy(); + config.rtp.protected_by_flexfec = (flexfec_stream_ != nullptr); + stream_ = call_->CreateVideoReceiveStream(std::move(config)); + stream_->Start(); +} + +void WebRtcVideoChannel2::WebRtcVideoReceiveStream:: + MaybeRecreateWebRtcFlexfecStream() { if (flexfec_stream_) { call_->DestroyFlexfecReceiveStream(flexfec_stream_); flexfec_stream_ = nullptr; } - const bool use_flexfec = flexfec_config_.IsCompleteAndEnabled(); - // TODO(nisse): There are way too many copies here. And why isn't - // the argument to CreateVideoReceiveStream a const ref? - webrtc::VideoReceiveStream::Config config = config_.Copy(); - config.rtp.protected_by_flexfec = use_flexfec; - stream_ = call_->CreateVideoReceiveStream(config.Copy()); - stream_->Start(); - - if (use_flexfec) { + if (flexfec_config_.IsCompleteAndEnabled()) { flexfec_stream_ = call_->CreateFlexfecReceiveStream(flexfec_config_); flexfec_stream_->Start(); } @@ -2523,6 +2566,13 @@ bool WebRtcVideoChannel2::VideoCodecSettings::operator==( rtx_payload_type == other.rtx_payload_type; } +bool WebRtcVideoChannel2::VideoCodecSettings::EqualsDisregardingFlexfec( + const WebRtcVideoChannel2::VideoCodecSettings& a, + const WebRtcVideoChannel2::VideoCodecSettings& b) { + return a.codec == b.codec && a.ulpfec == b.ulpfec && + a.rtx_payload_type == b.rtx_payload_type; +} + bool WebRtcVideoChannel2::VideoCodecSettings::operator!=( const WebRtcVideoChannel2::VideoCodecSettings& other) const { return !(*this == other); diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h index c40014a39e..394646bc32 100644 --- a/webrtc/media/engine/webrtcvideoengine2.h +++ b/webrtc/media/engine/webrtcvideoengine2.h @@ -191,9 +191,16 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { struct VideoCodecSettings { VideoCodecSettings(); + // Checks if all members of |*this| are equal to the corresponding members + // of |other|. bool operator==(const VideoCodecSettings& other) const; bool operator!=(const VideoCodecSettings& other) const; + // Checks if all members of |a|, except |flexfec_payload_type|, are equal + // to the corresponding members of |b|. + static bool EqualsDisregardingFlexfec(const VideoCodecSettings& a, + const VideoCodecSettings& b); + VideoCodec codec; webrtc::UlpfecConfig ulpfec; int flexfec_payload_type; @@ -213,6 +220,10 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { // These optionals are unset if not changed. rtc::Optional> codec_settings; rtc::Optional> rtp_header_extensions; + // Keep track of the FlexFEC payload type separately from |codec_settings|. + // This allows us to recreate the FlexfecReceiveStream separately from the + // VideoReceiveStream when the FlexFEC payload type is changed. + rtc::Optional flexfec_payload_type; }; bool GetChangedSendParameters(const VideoSendParameters& params, @@ -407,10 +418,12 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { bool external; }; - void RecreateWebRtcStream(); + void RecreateWebRtcVideoStream(); + void MaybeRecreateWebRtcFlexfecStream(); void ConfigureCodecs(const std::vector& recv_codecs, std::vector* old_codecs); + void ConfigureFlexfecCodec(int flexfec_payload_type); AllocatedDecoder CreateOrReuseVideoDecoder( std::vector* old_decoder, const VideoCodec& codec); @@ -460,8 +473,9 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { rtc::Optional SelectSendVideoCodec( const std::vector& remote_mapped_codecs) const; - static bool ReceiveCodecsHaveChanged(std::vector before, - std::vector after); + static bool NonFlexfecReceiveCodecsHaveChanged( + std::vector before, + std::vector after); void FillSenderStats(VideoMediaInfo* info, bool log_stats); void FillReceiverStats(VideoMediaInfo* info, bool log_stats); @@ -496,6 +510,9 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { WebRtcVideoDecoderFactory* const external_decoder_factory_; std::vector recv_codecs_; std::vector recv_rtp_extensions_; + // See reason for keeping track of the FlexFEC payload type separately in + // comment in WebRtcVideoChannel2::ChangedRecvParameters. + int recv_flexfec_payload_type_; webrtc::Call::Config::BitrateConfig bitrate_config_; // TODO(deadbeef): Don't duplicate information between // send_params/recv_params, rtp_extensions, options, etc. diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index 43dbbc6d18..95c37a1775 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -2510,6 +2510,53 @@ TEST_F(WebRtcVideoChannel2FlexfecRecvTest, SetDefaultRecvCodecsWithSsrc) { EXPECT_EQ(kSsrcs1[0], config.protected_media_ssrcs[0]); } +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, + EnablingFlexfecDoesNotRecreateVideoReceiveStream) { + cricket::VideoRecvParameters recv_parameters; + recv_parameters.codecs.push_back(GetEngineCodec("VP8")); + ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); + + AddRecvStream( + CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); + EXPECT_EQ(1, fake_call_->GetNumCreatedReceiveStreams()); + EXPECT_EQ(1U, fake_call_->GetVideoReceiveStreams().size()); + + // Enable FlexFEC. + recv_parameters.codecs.push_back(GetEngineCodec("flexfec-03")); + ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); + EXPECT_EQ(2, fake_call_->GetNumCreatedReceiveStreams()) + << "Enabling FlexFEC should create FlexfecReceiveStream."; + EXPECT_EQ(1U, fake_call_->GetVideoReceiveStreams().size()) + << "Enabling FlexFEC should not create VideoReceiveStream."; + EXPECT_EQ(1U, fake_call_->GetFlexfecReceiveStreams().size()) + << "Enabling FlexFEC should create a single FlexfecReceiveStream."; +} + +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, + DisablingFlexfecDoesNotRecreateVideoReceiveStream) { + cricket::VideoRecvParameters recv_parameters; + recv_parameters.codecs.push_back(GetEngineCodec("VP8")); + recv_parameters.codecs.push_back(GetEngineCodec("flexfec-03")); + ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); + + AddRecvStream( + CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); + EXPECT_EQ(2, fake_call_->GetNumCreatedReceiveStreams()); + EXPECT_EQ(1U, fake_call_->GetVideoReceiveStreams().size()); + EXPECT_EQ(1U, fake_call_->GetFlexfecReceiveStreams().size()); + + // Disable FlexFEC. + recv_parameters.codecs.clear(); + recv_parameters.codecs.push_back(GetEngineCodec("VP8")); + ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); + EXPECT_EQ(2, fake_call_->GetNumCreatedReceiveStreams()) + << "Disabling FlexFEC should not recreate VideoReceiveStream."; + EXPECT_EQ(1U, fake_call_->GetVideoReceiveStreams().size()) + << "Disabling FlexFEC should not destroy VideoReceiveStream."; + EXPECT_TRUE(fake_call_->GetFlexfecReceiveStreams().empty()) + << "Disabling FlexFEC should destroy FlexfecReceiveStream."; +} + // TODO(brandtr): When FlexFEC is no longer behind a field trial, merge all // tests that use this test fixture into the corresponding "non-field trial" // tests.