From dc69fd2b80c9452bc66d8fccea7bc2ae58b29558 Mon Sep 17 00:00:00 2001 From: Marina Ciocea Date: Fri, 10 Apr 2020 20:19:14 +0200 Subject: [PATCH] [InsertableStreams] Fix video sender simulcast. The transformer was previously moved into the config of the first stream which resulted in incorrect behavior for simulcast. Use the transformer in all the streams. Pass the sender's ssrc on registring the transformed frame callback, to associate separate transformer sinks for each sender. Bug: chromium:1065838 Change-Id: I5c52dacb241c68268681b85f875257b24987849e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/173332 Commit-Queue: Marina Ciocea Reviewed-by: Tommi Cr-Commit-Position: refs/heads/master@{#31050} --- call/rtp_video_sender.cc | 2 +- call/rtp_video_sender_unittest.cc | 46 +++++++++++++++++-- modules/rtp_rtcp/source/rtp_sender_video.cc | 6 ++- ...sender_video_frame_transformer_delegate.cc | 20 ++++---- ..._sender_video_frame_transformer_delegate.h | 7 +-- .../source/rtp_sender_video_unittest.cc | 10 ++-- 6 files changed, 68 insertions(+), 23 deletions(-) diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index cb32085d35..3a6a27cc7a 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -273,7 +273,7 @@ std::vector CreateRtpStreamSenders( rtp_config.ulpfec.red_payload_type != -1) { video_config.red_payload_type = rtp_config.ulpfec.red_payload_type; } - video_config.frame_transformer = std::move(frame_transformer); + video_config.frame_transformer = frame_transformer; auto sender_video = std::make_unique(video_config); rtp_streams.emplace_back(std::move(rtp_rtcp), std::move(sender_video), std::move(fec_generator)); diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index 71bec5e7bb..951cd4e410 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc @@ -117,7 +117,8 @@ class RtpVideoSenderTestFixture { const std::vector& rtx_ssrcs, int payload_type, const std::map& suspended_payload_states, - FrameCountObserver* frame_count_observer) + FrameCountObserver* frame_count_observer, + rtc::scoped_refptr frame_transformer) : time_controller_(Timestamp::Millis(1000000)), config_(CreateVideoSendStreamConfig(&transport_, ssrcs, @@ -151,8 +152,22 @@ class RtpVideoSenderTestFixture { &send_delay_stats_), &transport_controller_, &event_log_, &retransmission_rate_limiter_, std::make_unique(time_controller_.GetClock()), - nullptr, CryptoOptions{}, nullptr); + nullptr, CryptoOptions{}, frame_transformer); } + + RtpVideoSenderTestFixture( + const std::vector& ssrcs, + const std::vector& rtx_ssrcs, + int payload_type, + const std::map& suspended_payload_states, + FrameCountObserver* frame_count_observer) + : RtpVideoSenderTestFixture(ssrcs, + rtx_ssrcs, + payload_type, + suspended_payload_states, + frame_count_observer, + /*frame_transformer=*/nullptr) {} + RtpVideoSenderTestFixture( const std::vector& ssrcs, const std::vector& rtx_ssrcs, @@ -162,7 +177,8 @@ class RtpVideoSenderTestFixture { rtx_ssrcs, payload_type, suspended_payload_states, - /*frame_count_observer=*/nullptr) {} + /*frame_count_observer=*/nullptr, + /*frame_transformer=*/nullptr) {} RtpVideoSender* router() { return router_.get(); } MockTransport& transport() { return transport_; } @@ -801,4 +817,28 @@ TEST(RtpVideoSenderTest, CanSetZeroBitrateWithoutOverhead) { test.router()->OnBitrateUpdated(update, /*framerate*/ 0); } + +TEST(RtpVideoSenderTest, SimulcastSenderRegistersFrameTransformers) { + class MockFrameTransformer : public FrameTransformerInterface { + public: + MOCK_METHOD3(TransformFrame, + void(std::unique_ptr frame, + std::vector additional_data, + uint32_t ssrc)); + MOCK_METHOD2(RegisterTransformedFrameSinkCallback, + void(rtc::scoped_refptr, uint32_t)); + MOCK_METHOD1(UnregisterTransformedFrameSinkCallback, void(uint32_t)); + }; + + rtc::scoped_refptr transformer = + new rtc::RefCountedObject(); + + EXPECT_CALL(*transformer, RegisterTransformedFrameSinkCallback(_, kSsrc1)); + EXPECT_CALL(*transformer, RegisterTransformedFrameSinkCallback(_, kSsrc2)); + RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2}, + kPayloadType, {}, nullptr, transformer); + + EXPECT_CALL(*transformer, UnregisterTransformedFrameSinkCallback(kSsrc1)); + EXPECT_CALL(*transformer, UnregisterTransformedFrameSinkCallback(kSsrc2)); +} } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index fe7b724b04..0b6ee65307 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -160,7 +160,9 @@ RTPSenderVideo::RTPSenderVideo(const Config& config) config.frame_transformer ? new rtc::RefCountedObject< RTPSenderVideoFrameTransformerDelegate>( - this, std::move(config.frame_transformer)) + this, + config.frame_transformer, + rtp_sender_->SSRC()) : nullptr) { if (frame_transformer_delegate_) frame_transformer_delegate_->Init(); @@ -713,7 +715,7 @@ bool RTPSenderVideo::SendEncodedImage( // The frame will be sent async once transformed. return frame_transformer_delegate_->TransformFrame( payload_type, codec_type, rtp_timestamp, encoded_image, fragmentation, - video_header, expected_retransmission_time_ms, rtp_sender_->SSRC()); + video_header, expected_retransmission_time_ms); } return SendVideo(payload_type, codec_type, rtp_timestamp, encoded_image.capture_time_ms_, encoded_image, fragmentation, diff --git a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc index fa8309bac7..7107a3d847 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc @@ -105,12 +105,15 @@ class TransformableVideoSenderFrame : public TransformableVideoFrameInterface { RTPSenderVideoFrameTransformerDelegate::RTPSenderVideoFrameTransformerDelegate( RTPSenderVideo* sender, - rtc::scoped_refptr frame_transformer) - : sender_(sender), frame_transformer_(std::move(frame_transformer)) {} + rtc::scoped_refptr frame_transformer, + uint32_t ssrc) + : sender_(sender), + frame_transformer_(std::move(frame_transformer)), + ssrc_(ssrc) {} void RTPSenderVideoFrameTransformerDelegate::Init() { - frame_transformer_->RegisterTransformedFrameCallback( - rtc::scoped_refptr(this)); + frame_transformer_->RegisterTransformedFrameSinkCallback( + rtc::scoped_refptr(this), ssrc_); } bool RTPSenderVideoFrameTransformerDelegate::TransformFrame( @@ -120,8 +123,7 @@ bool RTPSenderVideoFrameTransformerDelegate::TransformFrame( const EncodedImage& encoded_image, const RTPFragmentationHeader* fragmentation, RTPVideoHeader video_header, - absl::optional expected_retransmission_time_ms, - uint32_t ssrc) { + absl::optional expected_retransmission_time_ms) { if (!encoder_queue_) encoder_queue_ = TaskQueueBase::Current(); // TODO(bugs.webrtc.org/11380) remove once this version of TransformFrame() is @@ -131,10 +133,10 @@ bool RTPSenderVideoFrameTransformerDelegate::TransformFrame( encoded_image.GetEncodedData(), video_header, payload_type, codec_type, rtp_timestamp, encoded_image.capture_time_ms_, fragmentation, expected_retransmission_time_ms), - RtpDescriptorAuthentication(video_header), ssrc); + RtpDescriptorAuthentication(video_header), ssrc_); frame_transformer_->Transform(std::make_unique( encoded_image, video_header, payload_type, codec_type, rtp_timestamp, - fragmentation, expected_retransmission_time_ms, ssrc)); + fragmentation, expected_retransmission_time_ms, ssrc_)); return true; } @@ -212,7 +214,7 @@ void RTPSenderVideoFrameTransformerDelegate::SetVideoStructureUnderLock( } void RTPSenderVideoFrameTransformerDelegate::Reset() { - frame_transformer_->UnregisterTransformedFrameCallback(); + frame_transformer_->UnregisterTransformedFrameSinkCallback(ssrc_); frame_transformer_ = nullptr; { rtc::CritScope lock(&sender_lock_); diff --git a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.h b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.h index 4a6482b2de..4c4713fe99 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.h +++ b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.h @@ -30,7 +30,8 @@ class RTPSenderVideoFrameTransformerDelegate : public TransformedFrameCallback { public: RTPSenderVideoFrameTransformerDelegate( RTPSenderVideo* sender, - rtc::scoped_refptr frame_transformer); + rtc::scoped_refptr frame_transformer, + uint32_t ssrc); void Init(); @@ -41,8 +42,7 @@ class RTPSenderVideoFrameTransformerDelegate : public TransformedFrameCallback { const EncodedImage& encoded_image, const RTPFragmentationHeader* fragmentation, RTPVideoHeader video_header, - absl::optional expected_retransmission_time_ms, - uint32_t ssrc); + absl::optional expected_retransmission_time_ms); // Implements TransformedFrameCallback. Can be called on any thread. Posts // the transformed frame to be sent on the |encoder_queue_|. @@ -71,6 +71,7 @@ class RTPSenderVideoFrameTransformerDelegate : public TransformedFrameCallback { rtc::CriticalSection sender_lock_; RTPSenderVideo* sender_ RTC_GUARDED_BY(sender_lock_); rtc::scoped_refptr frame_transformer_; + const uint32_t ssrc_; TaskQueueBase* encoder_queue_ = nullptr; }; diff --git a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc index 25f1b80551..6799fe9573 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc @@ -879,9 +879,9 @@ class MockFrameTransformer : public FrameTransformerInterface { void(std::unique_ptr frame, std::vector additional_data, uint32_t ssrc)); - MOCK_METHOD1(RegisterTransformedFrameCallback, - void(rtc::scoped_refptr)); - MOCK_METHOD0(UnregisterTransformedFrameCallback, void()); + MOCK_METHOD2(RegisterTransformedFrameSinkCallback, + void(rtc::scoped_refptr, uint32_t)); + MOCK_METHOD1(UnregisterTransformedFrameSinkCallback, void(uint32_t)); }; TEST_P(RtpSenderVideoTest, SendEncodedImageWithFrameTransformer) { @@ -893,7 +893,7 @@ TEST_P(RtpSenderVideoTest, SendEncodedImageWithFrameTransformer) { config.field_trials = &field_trials_; config.frame_transformer = transformer; - EXPECT_CALL(*transformer, RegisterTransformedFrameCallback(_)); + EXPECT_CALL(*transformer, RegisterTransformedFrameSinkCallback); std::unique_ptr rtp_sender_video = std::make_unique(config); @@ -908,7 +908,7 @@ TEST_P(RtpSenderVideoTest, SendEncodedImageWithFrameTransformer) { nullptr, hdr, kDefaultExpectedRetransmissionTimeMs); - EXPECT_CALL(*transformer, UnregisterTransformedFrameCallback()); + EXPECT_CALL(*transformer, UnregisterTransformedFrameSinkCallback); rtp_sender_video.reset(); }