From 43b15c3fc0d40c898c727695891c53c32f3bb766 Mon Sep 17 00:00:00 2001 From: Tommi Date: Thu, 28 Jul 2022 09:24:52 +0200 Subject: [PATCH] Add SetPayloadType to FlexfecReceiveStream. This reduces the number of times we recreate video receive streams and prepares for not having to do that for flexfec streams and avoid having to recreate a video receive stream when flexfec config changes. Bug: webrtc:11993 Change-Id: I11134b6a72eb162623ebbc12521d409da8616107 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261941 Reviewed-by: Danil Chapovalov Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#37641} --- call/flexfec_receive_stream.h | 4 +++ call/flexfec_receive_stream_impl.cc | 13 +++++++ call/flexfec_receive_stream_impl.h | 8 +++++ media/engine/fake_webrtc_call.h | 5 +++ media/engine/webrtc_video_engine.cc | 56 +++++++++++++++++++++++------ media/engine/webrtc_video_engine.h | 6 ++++ 6 files changed, 82 insertions(+), 10 deletions(-) diff --git a/call/flexfec_receive_stream.h b/call/flexfec_receive_stream.h index 7c8cd2ecbe..4f6fe44afa 100644 --- a/call/flexfec_receive_stream.h +++ b/call/flexfec_receive_stream.h @@ -65,6 +65,10 @@ class FlexfecReceiveStream : public RtpPacketSinkInterface, // Perhaps this should be in ReceiveStreamInterface and apply to audio streams // as well (although there's no logic that would use it at present). virtual void SetRtcpMode(RtcpMode mode) = 0; + + // Called to change the payload type after initialization. + virtual void SetPayloadType(int payload_type) = 0; + virtual int payload_type() const = 0; }; } // namespace webrtc diff --git a/call/flexfec_receive_stream_impl.cc b/call/flexfec_receive_stream_impl.cc index b962f670ae..9e00078f96 100644 --- a/call/flexfec_receive_stream_impl.cc +++ b/call/flexfec_receive_stream_impl.cc @@ -135,6 +135,7 @@ FlexfecReceiveStreamImpl::FlexfecReceiveStreamImpl( : extension_map_(std::move(config.rtp.extensions)), remote_ssrc_(config.rtp.remote_ssrc), transport_cc_(config.rtp.transport_cc), + payload_type_(config.payload_type), receiver_( MaybeCreateFlexfecReceiver(clock, config, recovered_packet_receiver)), rtp_receive_statistics_(ReceiveStatistics::Create(clock)), @@ -143,6 +144,7 @@ FlexfecReceiveStreamImpl::FlexfecReceiveStreamImpl( config, rtt_stats)) { RTC_LOG(LS_INFO) << "FlexfecReceiveStreamImpl: " << config.ToString(); + RTC_DCHECK_GE(payload_type_, -1); packet_sequence_checker_.Detach(); @@ -188,6 +190,17 @@ void FlexfecReceiveStreamImpl::OnRtpPacket(const RtpPacketReceived& packet) { } } +void FlexfecReceiveStreamImpl::SetPayloadType(int payload_type) { + RTC_DCHECK_RUN_ON(&packet_sequence_checker_); + RTC_DCHECK_GE(payload_type, -1); + payload_type_ = payload_type; +} + +int FlexfecReceiveStreamImpl::payload_type() const { + RTC_DCHECK_RUN_ON(&packet_sequence_checker_); + return payload_type_; +} + void FlexfecReceiveStreamImpl::SetRtpExtensions( std::vector extensions) { RTC_DCHECK_RUN_ON(&packet_sequence_checker_); diff --git a/call/flexfec_receive_stream_impl.h b/call/flexfec_receive_stream_impl.h index 21aef795fa..9cb383afee 100644 --- a/call/flexfec_receive_stream_impl.h +++ b/call/flexfec_receive_stream_impl.h @@ -55,6 +55,10 @@ class FlexfecReceiveStreamImpl : public FlexfecReceiveStream { // RtpPacketSinkInterface. void OnRtpPacket(const RtpPacketReceived& packet) override; + + void SetPayloadType(int payload_type) override; + int payload_type() const override; + // ReceiveStreamInterface impl. void SetRtpExtensions(std::vector extensions) override; RtpHeaderExtensionMap GetRtpExtensionMap() const override; @@ -88,6 +92,10 @@ class FlexfecReceiveStreamImpl : public FlexfecReceiveStream { const uint32_t remote_ssrc_; bool transport_cc_ RTC_GUARDED_BY(packet_sequence_checker_); + // `payload_type_` is initially set to -1, indicating that FlexFec is + // disabled. + int payload_type_ RTC_GUARDED_BY(packet_sequence_checker_) = -1; + // Erasure code interfacing. const std::unique_ptr receiver_; diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h index 5a7fddc041..c8d56cbcdc 100644 --- a/media/engine/fake_webrtc_call.h +++ b/media/engine/fake_webrtc_call.h @@ -323,6 +323,11 @@ class FakeFlexfecReceiveStream final : public webrtc::FlexfecReceiveStream { } void SetRtcpMode(webrtc::RtcpMode mode) override { config_.rtcp_mode = mode; } + int payload_type() const override { return config_.payload_type; } + void SetPayloadType(int payload_type) override { + config_.payload_type = payload_type; + } + const webrtc::FlexfecReceiveStream::Config& GetConfig() const; uint32_t remote_ssrc() const { return config_.rtp.remote_ssrc; } diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 89bf1f57ad..f05202ac6d 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -3035,10 +3035,46 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFeedbackParameters( RecreateReceiveStream(); } +void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFlexFecPayload( + int payload_type, + bool& flexfec_needs_recreation) { + // 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 + // the network thread). + if (flexfec_stream_) { + if (flexfec_stream_->payload_type() == payload_type) { + RTC_DCHECK_EQ(flexfec_config_.payload_type, payload_type); + return; + } + + flexfec_config_.payload_type = payload_type; + flexfec_stream_->SetPayloadType(payload_type); + + if (payload_type == -1) { + // TODO(tommi): Delete `flexfec_stream_` and clear references to it from + // `stream_` without having to recreate `stream_`. + flexfec_needs_recreation = true; + } + } 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; + } + } else { + // Noop. No flexfec stream exists and "new" payload_type == -1. + RTC_DCHECK(!flexfec_config_.IsCompleteAndEnabled()); + flexfec_config_.payload_type = payload_type; + } +} + 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); } @@ -3056,17 +3092,17 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetRecvParameters( } } } - if (params.flexfec_payload_type) { - flexfec_config_.payload_type = *params.flexfec_payload_type; - // TODO(tommi): See if it is better to always have a flexfec stream object - // configured and instead of recreating the video stream, reconfigure the - // flexfec object from within the rtp callback (soon to be on the network - // thread). - if (flexfec_stream_ || flexfec_config_.IsCompleteAndEnabled()) - video_needs_recreation = true; - } - if (video_needs_recreation) { + + if (params.flexfec_payload_type) + SetFlexFecPayload(*params.flexfec_payload_type, flexfec_needs_recreation); + + // 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) { 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 e080edafb3..bce5e055e6 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -505,6 +505,12 @@ 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); + void RecreateReceiveStream(); // Applies a new receive codecs configration to `config_`. Returns true