From 096427e494c6845848733b7605b125ea518ca813 Mon Sep 17 00:00:00 2001 From: Tony Herre Date: Fri, 28 Apr 2023 10:22:37 +0000 Subject: [PATCH] Overwrite frame seq nums when piping encoded frames between RTPReceivers This allows encoded frames to be written to any encoded insertable streams writer without needing to somehow set valid RTP sequence numbers. Assumes streams are using the Dependency Descriptor header ext. A short term fix while we discuss whether we can remove the sequence number check in RtpFrameReferenceFinder::ManageFrame. Bug: chromium:1439799 Change-Id: I3c1d83793cd8b6cae2a8ad2129b3b6daab1d11c4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/302301 Reviewed-by: Stefan Holmer Commit-Queue: Tony Herre Cr-Commit-Position: refs/heads/main@{#39966} --- modules/rtp_rtcp/source/frame_object.h | 5 ++ ...eam_receiver_frame_transformer_delegate.cc | 29 +++++++-- ...ver_frame_transformer_delegate_unittest.cc | 61 ++++++++++++++++++- 3 files changed, 89 insertions(+), 6 deletions(-) diff --git a/modules/rtp_rtcp/source/frame_object.h b/modules/rtp_rtcp/source/frame_object.h index ea37d18d13..2cbb932765 100644 --- a/modules/rtp_rtcp/source/frame_object.h +++ b/modules/rtp_rtcp/source/frame_object.h @@ -53,6 +53,11 @@ class RtpFrameObject : public EncodedFrame { const std::vector& Csrcs() const { return csrcs_; } + void SetFirstSeqNum(uint16_t first_seq_num) { + first_seq_num_ = first_seq_num; + } + void SetLastSeqNum(uint16_t last_seq_num) { last_seq_num_ = last_seq_num; } + private: // Reference for mutable access. rtc::scoped_refptr image_buffer_; diff --git a/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate.cc b/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate.cc index 6e915bf758..aa5c8dfa28 100644 --- a/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate.cc +++ b/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate.cc @@ -25,9 +25,11 @@ class TransformableVideoReceiverFrame : public TransformableVideoFrameInterface { public: TransformableVideoReceiverFrame(std::unique_ptr frame, - uint32_t ssrc) + uint32_t ssrc, + RtpVideoFrameReceiver* receiver) : frame_(std::move(frame)), - metadata_(frame_->GetRtpVideoHeader().GetAsMetadata()) { + metadata_(frame_->GetRtpVideoHeader().GetAsMetadata()), + receiver_(receiver) { metadata_.SetSsrc(ssrc); metadata_.SetCsrcs(frame_->Csrcs()); } @@ -64,9 +66,12 @@ class TransformableVideoReceiverFrame Direction GetDirection() const override { return Direction::kReceiver; } + const RtpVideoFrameReceiver* Receiver() { return receiver_; } + private: std::unique_ptr frame_; VideoFrameMetadata metadata_; + RtpVideoFrameReceiver* receiver_; }; } // namespace @@ -98,8 +103,8 @@ void RtpVideoStreamReceiverFrameTransformerDelegate::TransformFrame( std::unique_ptr frame) { RTC_DCHECK_RUN_ON(&network_sequence_checker_); frame_transformer_->Transform( - std::make_unique(std::move(frame), - ssrc_)); + std::make_unique(std::move(frame), ssrc_, + receiver_)); } void RtpVideoStreamReceiverFrameTransformerDelegate::OnTransformedFrame( @@ -121,7 +126,21 @@ void RtpVideoStreamReceiverFrameTransformerDelegate::ManageFrame( TransformableFrameInterface::Direction::kReceiver) { auto transformed_frame = absl::WrapUnique( static_cast(frame.release())); - receiver_->ManageFrame(std::move(*transformed_frame).ExtractFrame()); + auto frame_receiver = transformed_frame->Receiver(); + std::unique_ptr frame_object = + std::move(*transformed_frame).ExtractFrame(); + if (frame_receiver != receiver_) { + // This frame was received by a different RtpReceiver instance, so has + // first and last sequence numbers which will be meaningless to our + // receiver_. Work around this by using the frame id as a surrogate value, + // same as when given a Sender frame below. + + // TODO(https://crbug.com/1250638): Change what happens after the encoded + // insertable stream insertion to not require RTP data. + frame_object->SetFirstSeqNum(frame_object->Id()); + frame_object->SetLastSeqNum(frame_object->Id()); + } + receiver_->ManageFrame(std::move(frame_object)); } else { RTC_CHECK_EQ(frame->GetDirection(), TransformableFrameInterface::Direction::kSender); diff --git a/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate_unittest.cc b/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate_unittest.cc index 0fdc503e46..e0b53742a4 100644 --- a/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate_unittest.cc @@ -35,13 +35,19 @@ using ::testing::NiceMock; using ::testing::Return; using ::testing::SaveArg; +const int kFirstSeqNum = 1; +const int kLastSeqNum = 2; + std::unique_ptr CreateRtpFrameObject( const RTPVideoHeader& video_header, std::vector csrcs) { RtpPacketInfo packet_info(/*ssrc=*/123, csrcs, /*rtc_timestamp=*/0, /*receive_time=*/Timestamp::Seconds(123456)); return std::make_unique( - 0, 0, true, 0, 0, 0, 0, 0, VideoSendTiming(), 0, video_header.codec, + kFirstSeqNum, kLastSeqNum, /*markerBit=*/true, + /*times_nacked=*/3, /*first_packet_received_time=*/4, + /*last_packet_received_time=*/5, /*rtp_timestamp=*/6, /*ntp_time_ms=*/7, + VideoSendTiming(), /*payload_type=*/8, video_header.codec, kVideoRotation_0, VideoContentType::UNSPECIFIED, video_header, absl::nullopt, RtpPacketInfos({packet_info}), EncodedImageBuffer::Create(0)); @@ -121,6 +127,8 @@ TEST(RtpVideoStreamReceiverFrameTransformerDelegateTest, EXPECT_CALL(receiver, ManageFrame) .WillOnce([&](std::unique_ptr frame) { EXPECT_EQ(frame->Csrcs(), csrcs); + EXPECT_EQ(frame->first_seq_num(), kFirstSeqNum); + EXPECT_EQ(frame->last_seq_num(), kLastSeqNum); }); ON_CALL(*mock_frame_transformer, Transform) .WillByDefault( @@ -215,5 +223,56 @@ TEST(RtpVideoStreamReceiverFrameTransformerDelegateTest, rtc::ThreadManager::ProcessAllMessageQueuesForTesting(); } +TEST(RtpVideoStreamReceiverFrameTransformerDelegateTest, + ManageFrameFromDifferentReceiver) { + rtc::AutoThread main_thread_; + std::vector csrcs = {234, 345, 456}; + const int frame_id = 11; + + TestRtpVideoFrameReceiver receiver1; + auto mock_frame_transformer1( + rtc::make_ref_counted>()); + auto delegate1 = + rtc::make_ref_counted( + &receiver1, mock_frame_transformer1, rtc::Thread::Current(), + /*remote_ssrc*/ 1111); + + TestRtpVideoFrameReceiver receiver2; + auto mock_frame_transformer2( + rtc::make_ref_counted>()); + auto delegate2 = + rtc::make_ref_counted( + &receiver2, mock_frame_transformer2, rtc::Thread::Current(), + /*remote_ssrc*/ 1111); + + delegate1->Init(); + rtc::scoped_refptr callback_for_2; + EXPECT_CALL(*mock_frame_transformer2, RegisterTransformedFrameSinkCallback) + .WillOnce(SaveArg<0>(&callback_for_2)); + delegate2->Init(); + ASSERT_TRUE(callback_for_2); + + // Expect a call on receiver2's ManageFrame with sequence numbers overwritten + // with the frame's ID. + EXPECT_CALL(receiver2, ManageFrame) + .WillOnce([&](std::unique_ptr frame) { + EXPECT_EQ(frame->Csrcs(), csrcs); + EXPECT_EQ(frame->first_seq_num(), frame_id); + EXPECT_EQ(frame->last_seq_num(), frame_id); + }); + // When the frame transformer for receiver 1 receives the frame to transform, + // pipe it over to the callback for receiver 2. + ON_CALL(*mock_frame_transformer1, Transform) + .WillByDefault([&callback_for_2]( + std::unique_ptr frame) { + callback_for_2->OnTransformedFrame(std::move(frame)); + }); + std::unique_ptr untransformed_frame = + CreateRtpFrameObject(RTPVideoHeader(), csrcs); + untransformed_frame->SetId(frame_id); + delegate1->TransformFrame(std::move(untransformed_frame)); + rtc::ThreadManager::ProcessAllMessageQueuesForTesting(); +} + } // namespace } // namespace webrtc