From dfc4d81fd3cdff34c348a9e4bfe09bbaa667b22c Mon Sep 17 00:00:00 2001 From: Tommi Date: Tue, 2 Aug 2022 14:37:53 +0200 Subject: [PATCH] [WebRtcVideoReceiveStream] Construct flexfec stream on demand. ...for payload type changes and avoid recreating the main video stream. Bug: webrtc:11993 Change-Id: I03e44ff25d88caeb082a2e44b2e802d3b9392feb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/269244 Reviewed-by: Danil Chapovalov Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#37666} --- media/engine/webrtc_video_engine.cc | 16 +++++----------- media/engine/webrtc_video_engine.h | 9 ++++----- media/engine/webrtc_video_engine_unittest.cc | 19 ++++++++++--------- 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 4f02eab82d..1c61021c3f 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -3044,8 +3044,7 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFeedbackParameters( } void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFlexFecPayload( - int payload_type, - bool& flexfec_needs_recreation) { + int payload_type) { // TODO(bugs.webrtc.org/11993, tommi): See if it is better to always have a // flexfec stream object around and instead of recreating the video stream, // reconfigure the flexfec object from within the rtp callback (soon to be on @@ -3067,9 +3066,8 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFlexFecPayload( } else if (payload_type != -1) { flexfec_config_.payload_type = payload_type; if (flexfec_config_.IsCompleteAndEnabled()) { - // TODO(tommi): Construct new `flexfec_stream_` configure `stream_` - // without having to recreate `stream_`. - flexfec_needs_recreation = true; + flexfec_stream_ = call_->CreateFlexfecReceiveStream(flexfec_config_); + stream_->SetFlexFecProtection(flexfec_stream_); } } else { // Noop. No flexfec stream exists and "new" payload_type == -1. @@ -3082,7 +3080,6 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetRecvParameters( const ChangedRecvParameters& params) { RTC_DCHECK(stream_); bool video_needs_recreation = false; - bool flexfec_needs_recreation = false; if (params.codec_settings) { video_needs_recreation = ConfigureCodecs(*params.codec_settings); } @@ -3102,12 +3099,9 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetRecvParameters( } if (params.flexfec_payload_type) - SetFlexFecPayload(*params.flexfec_payload_type, flexfec_needs_recreation); + SetFlexFecPayload(*params.flexfec_payload_type); - // TODO(tommi): When `flexfec_needs_recreation` is `true` and - // `video_needs_recreation` is `false`, recreate only the flexfec stream and - // reconfigure the existing `stream_`. - if (video_needs_recreation || flexfec_needs_recreation) { + if (video_needs_recreation) { RecreateReceiveStream(); } else { RTC_DLOG_F(LS_INFO) << "No receive stream recreate needed."; diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index bce5e055e6..6775b08356 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -505,11 +505,10 @@ class WebRtcVideoChannel : public VideoMediaChannel, void SetLocalSsrc(uint32_t local_ssrc); private: - // Attempts to reconfigure an already existing `flexfec_stream_` or sets - // `flexfec_needs_recreation` to `true` if a new instance needs to be - // created by calling `RecreateReceiveStream()`. In all cases - // `SetFlexFecPayload()` will update `flexfec_config_`. - void SetFlexFecPayload(int payload_type, bool& flexfec_needs_recreation); + // Attempts to reconfigure an already existing `flexfec_stream_`, create + // one if the configuration is now complete or remove a flexfec stream + // when disabled. + void SetFlexFecPayload(int payload_type); void RecreateReceiveStream(); diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 0e1f2467ba..646a5d9d2d 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -4191,10 +4191,11 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, SetDefaultRecvCodecsWithSsrc) { } // Test changing the configuration after a video stream has been created and -// turn on flexfec. This will result in the video stream being recreated because -// the flexfec stream pointer is injected to the video stream at construction. +// turn on flexfec. This will result in video stream being reconfigured but not +// recreated because the flexfec stream pointer will be given to the already +// existing video stream instance. TEST_F(WebRtcVideoChannelFlexfecRecvTest, - EnablingFlexfecRecreatesVideoReceiveStream) { + EnablingFlexfecDoesNotRecreateVideoReceiveStream) { cricket::VideoRecvParameters recv_parameters; recv_parameters.codecs.push_back(GetEngineCodec("VP8")); ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); @@ -4215,13 +4216,13 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, recv_parameters.codecs.push_back(GetEngineCodec("flexfec-03")); ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); - // Now the count of created streams will be 3 since the video stream was - // recreated and a flexfec stream was created. - EXPECT_EQ(3, fake_call_->GetNumCreatedReceiveStreams()) - << "Enabling FlexFEC should create FlexfecReceiveStream."; - + // The count of created streams will remain 2 despite the creation of a new + // flexfec stream. The existing receive stream will have been reconfigured + // to use the new flexfec instance. + EXPECT_EQ(2, fake_call_->GetNumCreatedReceiveStreams()) + << "Enabling FlexFEC should not create VideoReceiveStreamInterface (1)."; EXPECT_EQ(1U, fake_call_->GetVideoReceiveStreams().size()) - << "Enabling FlexFEC should not create VideoReceiveStreamInterface."; + << "Enabling FlexFEC should not create VideoReceiveStreamInterface (2)."; EXPECT_EQ(1U, fake_call_->GetFlexfecReceiveStreams().size()) << "Enabling FlexFEC should create a single FlexfecReceiveStream."; video_stream = video_streams.front();