Use scoped_refptr for RtpSenderInterface::SetEncoderSelector To improve memory safety and simplify lifetime management of VideoEncoderFactory::EncoderSelectorInterface, this change refactors its ownership model to use reference counting. This change ensures the encoder selector remains valid as long as it is in use. Bug: webrtc:42224373, b/419314207 Change-Id: Iaf620651435f261aeb0cdb98c779aef5eda4610a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/462526 Reviewed-by: Danil Chapovalov <danilchap@webrtc.org> Commit-Queue: Per Kjellander <perkj@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47378}
diff --git a/api/rtp_sender_interface.h b/api/rtp_sender_interface.h index 5ea6efe..32cbfeb 100644 --- a/api/rtp_sender_interface.h +++ b/api/rtp_sender_interface.h
@@ -128,9 +128,21 @@ // Sets a user defined encoder selector. // Overrides selector that is (optionally) provided by VideoEncoderFactory. + [[deprecated( + "Use SetEncoderSelector with Ref Counted EncoderSelectorInterface")]] virtual void SetEncoderSelector( std::unique_ptr<VideoEncoderFactory::EncoderSelectorInterface> - encoder_selector) = 0; + encoder_selector) { + SetEncoderSelector( + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface>( + encoder_selector.release())); + } + + virtual void SetEncoderSelector( + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector) { + RTC_DCHECK_NOTREACHED(); + } // Default implementation of SetFrameTransformer. // TODO: bugs.webrtc.org/15929 - remove when all implementations are good
diff --git a/api/test/mock_rtpsender.h b/api/test/mock_rtpsender.h index 59da465..08a62e3 100644 --- a/api/test/mock_rtpsender.h +++ b/api/test/mock_rtpsender.h
@@ -12,7 +12,6 @@ #define API_TEST_MOCK_RTPSENDER_H_ #include <cstdint> -#include <memory> #include <string> #include <type_traits> #include <vector> @@ -82,7 +81,7 @@ (override)); MOCK_METHOD(void, SetEncoderSelector, - (std::unique_ptr<VideoEncoderFactory::EncoderSelectorInterface>), + (scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface>), (override)); MOCK_METHOD(void, SetObserver, (RtpSenderObserverInterface*), (override)); };
diff --git a/api/video_codecs/BUILD.gn b/api/video_codecs/BUILD.gn index 0afffa1..583ade7 100644 --- a/api/video_codecs/BUILD.gn +++ b/api/video_codecs/BUILD.gn
@@ -80,6 +80,7 @@ deps = [ ":scalability_mode", "..:fec_controller_api", + "..:refcountedbase", "..:rtp_parameters", "../../media:media_constants", "../../modules/video_coding:codec_globals_headers",
diff --git a/api/video_codecs/video_encoder_factory.h b/api/video_codecs/video_encoder_factory.h index 8986e920..ebc6e39 100644 --- a/api/video_codecs/video_encoder_factory.h +++ b/api/video_codecs/video_encoder_factory.h
@@ -17,6 +17,7 @@ #include <vector> #include "api/environment/environment.h" +#include "api/ref_counted_base.h" #include "api/units/data_rate.h" #include "api/video/render_resolution.h" #include "api/video_codecs/sdp_video_format.h" @@ -38,9 +39,13 @@ // typically stateful to avoid toggling between different encoders, which is // costly due to recreation of objects, a new codec will always start with a // key-frame. - class EncoderSelectorInterface { + // A note about threading: This interface is used on a dedicated encoder + // queue. But creation and deletion may happen on another thread. + class EncoderSelectorInterface : public RefCountedBase { public: - virtual ~EncoderSelectorInterface() {} + // TODO: bugs.webrtc.org/42224373 - make destructor protected once all + // implementations use reference counting. + virtual ~EncoderSelectorInterface() = default; // Informs the encoder selector about which encoder that is currently being // used. @@ -115,7 +120,7 @@ // recommended. // // TODO(bugs.webrtc.org:14122): Deprecate and remove in favor of - // `RtpSenderInterface::SetEncoderSelector`. + // `RtpSenderInterface::SetEncoderSelector` using ref count. virtual std::unique_ptr<EncoderSelectorInterface> GetEncoderSelector() const { return nullptr; }
diff --git a/call/video_send_stream.h b/call/video_send_stream.h index 683be5f..07a2635 100644 --- a/call/video_send_stream.h +++ b/call/video_send_stream.h
@@ -210,8 +210,8 @@ // An optional encoder selector provided by the user. // Overrides VideoEncoderFactory::GetEncoderSelector(). - // Owned by RtpSenderBase. - VideoEncoderFactory::EncoderSelectorInterface* encoder_selector = nullptr; + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector; // Per PeerConnection cryptography options. CryptoOptions crypto_options;
diff --git a/media/base/media_channel.h b/media/base/media_channel.h index 1e9d0d4..10ab6eb 100644 --- a/media/base/media_channel.h +++ b/media/base/media_channel.h
@@ -232,11 +232,10 @@ uint32_t ssrc, scoped_refptr<FrameTransformerInterface> frame_transformer) = 0; - // note: The encoder_selector object must remain valid for the lifetime of the - // MediaChannel, unless replaced. virtual void SetEncoderSelector( uint32_t /* ssrc */, - VideoEncoderFactory::EncoderSelectorInterface* /* encoder_selector */) {} + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + /*encoder_selector*/) {} virtual RtpParameters GetRtpSendParameters(uint32_t ssrc) const = 0; // Returns a callback that is to be used to retrieve RTP send parameters // in the same way as `GetRtpSendParameters` does.
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 65fd906..db5e34d 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc
@@ -1822,11 +1822,12 @@ void WebRtcVideoSendChannel::SetEncoderSelector( uint32_t ssrc, - VideoEncoderFactory::EncoderSelectorInterface* encoder_selector) { + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector) { RTC_DCHECK_RUN_ON(&thread_checker_); auto matching_stream = send_streams_.find(ssrc); if (matching_stream != send_streams_.end()) { - matching_stream->second->SetEncoderSelector(encoder_selector); + matching_stream->second->SetEncoderSelector(std::move(encoder_selector)); } else { RTC_LOG(LS_ERROR) << "No stream found to attach encoder selector"; } @@ -2284,9 +2285,10 @@ } void WebRtcVideoSendChannel::WebRtcVideoSendStream::SetEncoderSelector( - VideoEncoderFactory::EncoderSelectorInterface* encoder_selector) { + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector) { RTC_DCHECK_RUN_ON(&thread_checker_); - parameters_.config.encoder_selector = encoder_selector; + parameters_.config.encoder_selector = std::move(encoder_selector); if (stream_) { RTC_LOG(LS_INFO) << "RecreateWebRtcStream (send) because of SetEncoderSelector, ssrc="
diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index 45ff9a7..a08c919 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h
@@ -231,7 +231,8 @@ // MediaChannel, unless replaced. void SetEncoderSelector( uint32_t ssrc, - VideoEncoderFactory::EncoderSelectorInterface* encoder_selector) override; + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector) override; void SetSsrcListChangedCallback( absl::AnyInvocable<void(const std::set<uint32_t>&)> callback) override { @@ -336,7 +337,8 @@ // note: The encoder_selector object must remain valid for the lifetime of // the MediaChannel, unless replaced. void SetEncoderSelector( - VideoEncoderFactory::EncoderSelectorInterface* encoder_selector); + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector); void SetOptions(const VideoOptions& options);
diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index c9de2e5..cf7d4c5 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc
@@ -10030,11 +10030,12 @@ ASSERT_TRUE(codec); EXPECT_EQ("VP8", codec->name); - MockEncoderSelector encoder_selector; - EXPECT_CALL(encoder_selector, OnAvailableBitrate) + scoped_refptr<MockEncoderSelector> encoder_selector = + make_ref_counted<MockEncoderSelector>(); + EXPECT_CALL(*encoder_selector, OnAvailableBitrate) .WillRepeatedly(Return(SdpVideoFormat::VP9Profile0())); - send_channel_->SetEncoderSelector(kSsrc, &encoder_selector); + send_channel_->SetEncoderSelector(kSsrc, encoder_selector); time_controller_.AdvanceTime(kFrameDuration); codec = send_channel_->GetSendCodec();
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index a372bb6..e2f68f7 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc
@@ -4179,11 +4179,11 @@ IsRtcOk()); ASSERT_EQ(callee()->pc()->GetReceivers().size(), 1u); - std::unique_ptr<MockEncoderSelector> encoder_selector = - std::make_unique<MockEncoderSelector>(); + scoped_refptr<MockEncoderSelector> encoder_selector = + make_ref_counted<MockEncoderSelector>(); EXPECT_CALL(*encoder_selector, OnCurrentEncoder); - sender->SetEncoderSelector(std::move(encoder_selector)); + sender->SetEncoderSelector(encoder_selector); // Expect video to be received in one direction. MediaExpectations media_expectations; @@ -4210,8 +4210,8 @@ IsRtcOk()); ASSERT_EQ(callee()->pc()->GetReceivers().size(), 1u); - std::unique_ptr<MockEncoderSelector> encoder_selector = - std::make_unique<MockEncoderSelector>(); + scoped_refptr<MockEncoderSelector> encoder_selector = + make_ref_counted<MockEncoderSelector>(); std::optional<SdpVideoFormat> next_format; EXPECT_CALL(*encoder_selector, OnCurrentEncoder) .WillOnce([&](const SdpVideoFormat& format) { @@ -4223,7 +4223,7 @@ EXPECT_CALL(*encoder_selector, OnAvailableBitrate) .WillRepeatedly([&](const DataRate& rate) { return next_format; }); - sender->SetEncoderSelector(std::move(encoder_selector)); + sender->SetEncoderSelector(encoder_selector); // Expect video to be received in one direction. MediaExpectations media_expectations;
diff --git a/pc/peer_connection_rtp_unittest.cc b/pc/peer_connection_rtp_unittest.cc index f2fcba4..7f78238 100644 --- a/pc/peer_connection_rtp_unittest.cc +++ b/pc/peer_connection_rtp_unittest.cc
@@ -2115,12 +2115,13 @@ } }; - auto encoder_selector = std::make_unique<MockEncoderSelector>(); + scoped_refptr<MockEncoderSelector> encoder_selector = + make_ref_counted<MockEncoderSelector>(); auto format_to_switch_to = *encoder_selector->OnAvailableBitrate(DataRate::Zero()); // Change encoder selector - sender->SetEncoderSelector(std::move(encoder_selector)); + sender->SetEncoderSelector(encoder_selector); auto transceivers = caller->GetInternalPeerConnection()->GetTransceiversInternal();
diff --git a/pc/rtp_sender.cc b/pc/rtp_sender.cc index 7af007c..3edc511 100644 --- a/pc/rtp_sender.cc +++ b/pc/rtp_sender.cc
@@ -245,6 +245,15 @@ std::unique_ptr<VideoEncoderFactory::EncoderSelectorInterface> encoder_selector) { RTC_DCHECK_RUN_ON(signaling_thread_); + SetEncoderSelector( + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface>( + encoder_selector.release())); +} + +void RtpSenderBase::SetEncoderSelector( + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector) { + RTC_DCHECK_RUN_ON(signaling_thread_); encoder_selector_ = std::move(encoder_selector); SetEncoderSelectorOnChannel(); } @@ -258,7 +267,7 @@ worker_thread_->BlockingCall([&, ssrc = ssrc_] { RTC_DCHECK_RUN_ON(worker_thread_); if (media_channel_) - media_channel_->SetEncoderSelector(ssrc, encoder_selector_.get()); + media_channel_->SetEncoderSelector(ssrc, encoder_selector_); }); } @@ -778,7 +787,7 @@ ssrc, frame_transformer_); } if (encoder_selector_) { - media_channel_->SetEncoderSelector(ssrc, encoder_selector_.get()); + media_channel_->SetEncoderSelector(ssrc, encoder_selector_); } }); if (params_modified) {
diff --git a/pc/rtp_sender.h b/pc/rtp_sender.h index eeec9ae..a38b91e 100644 --- a/pc/rtp_sender.h +++ b/pc/rtp_sender.h
@@ -232,6 +232,10 @@ std::unique_ptr<VideoEncoderFactory::EncoderSelectorInterface> encoder_selector) override; + void SetEncoderSelector( + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector) override; + void SetEncoderSelectorOnChannel(); void SetSendCodecs(std::vector<Codec> send_codecs) override { @@ -334,8 +338,7 @@ bool sent_first_packet_ = false; scoped_refptr<FrameTransformerInterface> frame_transformer_; - // Guard with RTC_GUARDED_BY(worker_thread_) after refactoring. - std::unique_ptr<VideoEncoderFactory::EncoderSelectorInterface> + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> encoder_selector_; scoped_refptr<PendingTaskSafetyFlag> worker_safety_;
diff --git a/pc/rtp_sender_proxy.h b/pc/rtp_sender_proxy.h index 74d28b5..4aa6c57 100644 --- a/pc/rtp_sender_proxy.h +++ b/pc/rtp_sender_proxy.h
@@ -62,9 +62,15 @@ PROXY_METHOD1(RTCErrorOr<scoped_refptr<SframeEncrypterInterface>>, CreateSframeEncrypterOrError, const SframeEncrypterInit&) +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" PROXY_METHOD1(void, SetEncoderSelector, std::unique_ptr<VideoEncoderFactory::EncoderSelectorInterface>) +#pragma clang diagnostic pop +PROXY_METHOD1(void, + SetEncoderSelector, + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface>) PROXY_METHOD1(RTCError, GenerateKeyFrame, const std::vector<std::string>&) END_PROXY_MAP(RtpSender)
diff --git a/pc/test/mock_rtp_sender_internal.h b/pc/test/mock_rtp_sender_internal.h index eeba6fa..24f19bc 100644 --- a/pc/test/mock_rtp_sender_internal.h +++ b/pc/test/mock_rtp_sender_internal.h
@@ -109,6 +109,10 @@ SetEncoderSelector, (std::unique_ptr<VideoEncoderFactory::EncoderSelectorInterface>), (override)); + MOCK_METHOD(void, + SetEncoderSelector, + (scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface>), + (override)); MOCK_METHOD(void, SetObserver, (RtpSenderObserverInterface*), (override)); // RtpSenderInternal methods.
diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index f4d5d27..f1305b5 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc
@@ -391,7 +391,8 @@ VideoStreamEncoder::BitrateAllocationCallbackType bitrate_allocation_callback_type, Metronome* metronome, - VideoEncoderFactory::EncoderSelectorInterface* encoder_selector, + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector, EncoderSwitchRequestCallback encoder_switch_request_callback) { std::unique_ptr<TaskQueueBase, TaskQueueDeleter> encoder_queue = env.task_queue_factory().CreateTaskQueue( @@ -407,7 +408,7 @@ &env.clock(), encoder_queue_ptr, metronome, /*worker_queue=*/TaskQueueBase::Current(), env.field_trials()), std::move(encoder_queue), bitrate_allocation_callback_type, - encoder_selector, std::move(encoder_switch_request_callback)); + std::move(encoder_selector), std::move(encoder_switch_request_callback)); } bool HasActiveEncodings(const VideoEncoderConfig& config) {
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 87ddade..dccfa5b 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc
@@ -711,7 +711,8 @@ std::unique_ptr<FrameCadenceAdapterInterface> frame_cadence_adapter, std::unique_ptr<TaskQueueBase, TaskQueueDeleter> encoder_queue, BitrateAllocationCallbackType allocation_cb_type, - VideoEncoderFactory::EncoderSelectorInterface* encoder_selector, + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector, EncoderSwitchRequestCallback encoder_switch_request_callback) : env_(env), worker_queue_(TaskQueueBase::Current()), @@ -721,14 +722,12 @@ std::move(encoder_switch_request_callback)), allocation_cb_type_(allocation_cb_type), rate_control_settings_(env_.field_trials()), - encoder_selector_from_constructor_(encoder_selector), - encoder_selector_from_factory_( - encoder_selector_from_constructor_ - ? nullptr - : settings_.encoder_factory->GetEncoderSelector()), - encoder_selector_(encoder_selector_from_constructor_ - ? encoder_selector_from_constructor_ - : encoder_selector_from_factory_.get()), + encoder_selector_( + encoder_selector != nullptr ? std::move(encoder_selector) + : settings_.encoder_factory != nullptr + ? scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface>( + settings_.encoder_factory->GetEncoderSelector().release()) + : nullptr), encoder_stats_observer_(encoder_stats_observer), frame_cadence_adapter_(std::move(frame_cadence_adapter)), delta_ntp_internal_ms_(env_.clock().CurrentNtpInMilliseconds() -
diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h index f6c4ca7..04e6046 100644 --- a/video/video_stream_encoder.h +++ b/video/video_stream_encoder.h
@@ -98,7 +98,8 @@ std::unique_ptr<FrameCadenceAdapterInterface> frame_cadence_adapter, std::unique_ptr<TaskQueueBase, TaskQueueDeleter> encoder_queue, BitrateAllocationCallbackType allocation_cb_type, - VideoEncoderFactory::EncoderSelectorInterface* encoder_selector = nullptr, + scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector = nullptr, EncoderSwitchRequestCallback encoder_switch_request_callback = nullptr); ~VideoStreamEncoder() override; @@ -311,13 +312,10 @@ const BitrateAllocationCallbackType allocation_cb_type_; const RateControlSettings rate_control_settings_; - VideoEncoderFactory::EncoderSelectorInterface* const - encoder_selector_from_constructor_; - std::unique_ptr<VideoEncoderFactory::EncoderSelectorInterface> const - encoder_selector_from_factory_; - // Pointing to either encoder_selector_from_constructor_ or - // encoder_selector_from_factory_ but can be nullptr. - VideoEncoderFactory::EncoderSelectorInterface* const encoder_selector_; + // Pointing to either encoder_selector from constructor or + // encoder_selector from_factory_ but can be nullptr. + const scoped_refptr<VideoEncoderFactory::EncoderSelectorInterface> + encoder_selector_; VideoStreamEncoderObserver* const encoder_stats_observer_; // Adapter that avoids public inheritance of the cadence adapter's callback @@ -478,7 +476,7 @@ RTC_GUARDED_BY(encoder_queue_); // Used to cancel any potentially pending tasks to the worker thread. - // Refrenced by tasks running on `encoder_queue_` so need to be destroyed + // Referenced by tasks running on `encoder_queue_` so need to be destroyed // after stopping that queue. Must be created and destroyed on // `worker_queue_`. ScopedTaskSafety task_safety_;