From 36500ab6349a82356602489265f5ca9bc4ea890d Mon Sep 17 00:00:00 2001 From: Tony Herre Date: Tue, 29 Aug 2023 12:01:32 +0200 Subject: [PATCH] Move RTPTimestamp offset handling out of encoded transform delegate Keep the logic managing whether audio RTP timestamps have the random start offset added or not inside ChannelSend, so that the ChannelSendFrameTransformerDelegate doesn't need to worry about it. Crucially, this means that frames moved between senders by encoded transforms clients will always use the correct offset for the channel where we actually get sent. Also rename TS variables throughout both classes to be explicit over whether the offset has been added or not. Bug: chromium:1464847 Change-Id: I19955ec4c1cb834161b00dd74622725a070b713a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/317900 Commit-Queue: Tony Herre Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#40655} --- audio/channel_send.cc | 20 +++++---- ...channel_send_frame_transformer_delegate.cc | 38 +++++----------- .../channel_send_frame_transformer_delegate.h | 3 +- ...end_frame_transformer_delegate_unittest.cc | 8 ++-- audio/channel_send_unittest.cc | 43 +++++++++++++++++++ 5 files changed, 71 insertions(+), 41 deletions(-) diff --git a/audio/channel_send.cc b/audio/channel_send.cc index fccd58b76c..984f530d38 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -165,7 +165,7 @@ class ChannelSend : public ChannelSendInterface, int32_t SendRtpAudio(AudioFrameType frameType, uint8_t payloadType, - uint32_t rtp_timestamp, + uint32_t rtp_timestamp_without_offset, rtc::ArrayView payload, int64_t absolute_capture_timestamp_ms) RTC_RUN_ON(encoder_queue_); @@ -280,7 +280,7 @@ int32_t ChannelSend::SendData(AudioFrameType frameType, // Asynchronously transform the payload before sending it. After the payload // is transformed, the delegate will call SendRtpAudio to send it. frame_transformer_delegate_->Transform( - frameType, payloadType, rtp_timestamp, rtp_rtcp_->StartTimestamp(), + frameType, payloadType, rtp_timestamp + rtp_rtcp_->StartTimestamp(), payloadData, payloadSize, absolute_capture_timestamp_ms, rtp_rtcp_->SSRC()); return 0; @@ -291,7 +291,7 @@ int32_t ChannelSend::SendData(AudioFrameType frameType, int32_t ChannelSend::SendRtpAudio(AudioFrameType frameType, uint8_t payloadType, - uint32_t rtp_timestamp, + uint32_t rtp_timestamp_without_offset, rtc::ArrayView payload, int64_t absolute_capture_timestamp_ms) { if (include_audio_level_indication_.load()) { @@ -341,7 +341,7 @@ int32_t ChannelSend::SendRtpAudio(AudioFrameType frameType, // Push data from ACM to RTP/RTCP-module to deliver audio frame for // packetization. - if (!rtp_rtcp_->OnSendingRtpFrame(rtp_timestamp, + if (!rtp_rtcp_->OnSendingRtpFrame(rtp_timestamp_without_offset, // Leaving the time when this frame was // received from the capture device as // undefined for voice for now. @@ -358,7 +358,8 @@ int32_t ChannelSend::SendRtpAudio(AudioFrameType frameType, // This call will trigger Transport::SendPacket() from the RTP/RTCP module. if (!rtp_sender_audio_->SendAudio( - frameType, payloadType, rtp_timestamp + rtp_rtcp_->StartTimestamp(), + frameType, payloadType, + rtp_timestamp_without_offset + rtp_rtcp_->StartTimestamp(), payload.data(), payload.size(), absolute_capture_timestamp_ms)) { RTC_DLOG(LS_ERROR) << "ChannelSend::SendData() failed to send data to RTP/RTCP module"; @@ -841,11 +842,14 @@ void ChannelSend::InitFrameTransformerDelegate( // to send the transformed audio. ChannelSendFrameTransformerDelegate::SendFrameCallback send_audio_callback = [this](AudioFrameType frameType, uint8_t payloadType, - uint32_t rtp_timestamp, rtc::ArrayView payload, + uint32_t rtp_timestamp_with_offset, + rtc::ArrayView payload, int64_t absolute_capture_timestamp_ms) { RTC_DCHECK_RUN_ON(&encoder_queue_); - return SendRtpAudio(frameType, payloadType, rtp_timestamp, payload, - absolute_capture_timestamp_ms); + return SendRtpAudio( + frameType, payloadType, + rtp_timestamp_with_offset - rtp_rtcp_->StartTimestamp(), payload, + absolute_capture_timestamp_ms); }; frame_transformer_delegate_ = rtc::make_ref_counted( diff --git a/audio/channel_send_frame_transformer_delegate.cc b/audio/channel_send_frame_transformer_delegate.cc index e07e1a70c8..9b88d4d949 100644 --- a/audio/channel_send_frame_transformer_delegate.cc +++ b/audio/channel_send_frame_transformer_delegate.cc @@ -50,16 +50,14 @@ class TransformableOutgoingAudioFrame public: TransformableOutgoingAudioFrame(AudioFrameType frame_type, uint8_t payload_type, - uint32_t rtp_timestamp, - uint32_t rtp_start_timestamp, + uint32_t rtp_timestamp_with_offset, const uint8_t* payload_data, size_t payload_size, int64_t absolute_capture_timestamp_ms, uint32_t ssrc) : frame_type_(frame_type), payload_type_(payload_type), - rtp_timestamp_(rtp_timestamp), - rtp_start_timestamp_(rtp_start_timestamp), + rtp_timestamp_with_offset_(rtp_timestamp_with_offset), payload_(payload_data, payload_size), absolute_capture_timestamp_ms_(absolute_capture_timestamp_ms), ssrc_(ssrc) {} @@ -68,10 +66,7 @@ class TransformableOutgoingAudioFrame void SetData(rtc::ArrayView data) override { payload_.SetData(data.data(), data.size()); } - uint32_t GetTimestamp() const override { - return rtp_timestamp_ + rtp_start_timestamp_; - } - uint32_t GetStartTimestamp() const { return rtp_start_timestamp_; } + uint32_t GetTimestamp() const override { return rtp_timestamp_with_offset_; } uint32_t GetSsrc() const override { return ssrc_; } IfaceFrameType Type() const override { @@ -89,8 +84,8 @@ class TransformableOutgoingAudioFrame return absl::nullopt; } - void SetRTPTimestamp(uint32_t timestamp) override { - rtp_timestamp_ = timestamp - rtp_start_timestamp_; + void SetRTPTimestamp(uint32_t rtp_timestamp_with_offset) override { + rtp_timestamp_with_offset_ = rtp_timestamp_with_offset; } absl::optional AbsoluteCaptureTimestamp() const override { @@ -100,8 +95,7 @@ class TransformableOutgoingAudioFrame private: AudioFrameType frame_type_; uint8_t payload_type_; - uint32_t rtp_timestamp_; - uint32_t rtp_start_timestamp_; + uint32_t rtp_timestamp_with_offset_; rtc::Buffer payload_; int64_t absolute_capture_timestamp_ms_; uint32_t ssrc_; @@ -133,15 +127,14 @@ void ChannelSendFrameTransformerDelegate::Transform( AudioFrameType frame_type, uint8_t payload_type, uint32_t rtp_timestamp, - uint32_t rtp_start_timestamp, const uint8_t* payload_data, size_t payload_size, int64_t absolute_capture_timestamp_ms, uint32_t ssrc) { frame_transformer_->Transform( std::make_unique( - frame_type, payload_type, rtp_timestamp, rtp_start_timestamp, - payload_data, payload_size, absolute_capture_timestamp_ms, ssrc)); + frame_type, payload_type, rtp_timestamp, payload_data, payload_size, + absolute_capture_timestamp_ms, ssrc)); } void ChannelSendFrameTransformerDelegate::OnTransformedFrame( @@ -162,19 +155,11 @@ void ChannelSendFrameTransformerDelegate::SendFrame( RTC_DCHECK_RUN_ON(encoder_queue_); if (!send_frame_callback_) return; - uint32_t rtp_start_timestamp = 0; - if (frame->GetDirection() == - TransformableFrameInterface::Direction::kSender) { - auto* outgoing_frame = - static_cast(frame.get()); - rtp_start_timestamp = outgoing_frame->GetStartTimestamp(); - } auto* transformed_frame = static_cast(frame.get()); send_frame_callback_( InterfaceFrameTypeToInternalFrameType(transformed_frame->Type()), - transformed_frame->GetPayloadType(), - transformed_frame->GetTimestamp() - rtp_start_timestamp, + transformed_frame->GetPayloadType(), transformed_frame->GetTimestamp(), transformed_frame->GetData(), transformed_frame->AbsoluteCaptureTimestamp() ? *transformed_frame->AbsoluteCaptureTimestamp() @@ -187,9 +172,8 @@ std::unique_ptr CloneSenderAudioFrame( return std::make_unique( InterfaceFrameTypeToInternalFrameType(original->Type()), original->GetPayloadType(), original->GetTimestamp(), - /*rtp_start_timestamp=*/0u, original->GetData().data(), - original->GetData().size(), original->GetTimestamp(), - original->GetSsrc()); + original->GetData().data(), original->GetData().size(), + original->GetTimestamp(), original->GetSsrc()); } } // namespace webrtc diff --git a/audio/channel_send_frame_transformer_delegate.h b/audio/channel_send_frame_transformer_delegate.h index c866638547..eb0027e4c8 100644 --- a/audio/channel_send_frame_transformer_delegate.h +++ b/audio/channel_send_frame_transformer_delegate.h @@ -32,7 +32,7 @@ class ChannelSendFrameTransformerDelegate : public TransformedFrameCallback { using SendFrameCallback = std::function payload, int64_t absolute_capture_timestamp_ms)>; ChannelSendFrameTransformerDelegate( @@ -54,7 +54,6 @@ class ChannelSendFrameTransformerDelegate : public TransformedFrameCallback { void Transform(AudioFrameType frame_type, uint8_t payload_type, uint32_t rtp_timestamp, - uint32_t rtp_start_timestamp, const uint8_t* payload_data, size_t payload_size, int64_t absolute_capture_timestamp_ms, diff --git a/audio/channel_send_frame_transformer_delegate_unittest.cc b/audio/channel_send_frame_transformer_delegate_unittest.cc index 998a6e689a..f75d4a8ab7 100644 --- a/audio/channel_send_frame_transformer_delegate_unittest.cc +++ b/audio/channel_send_frame_transformer_delegate_unittest.cc @@ -114,8 +114,8 @@ TEST(ChannelSendFrameTransformerDelegateTest, [&callback](std::unique_ptr frame) { callback->OnTransformedFrame(std::move(frame)); }); - delegate->Transform(AudioFrameType::kEmptyFrame, 0, 0, 0, data, sizeof(data), - 0, 0); + delegate->Transform(AudioFrameType::kEmptyFrame, 0, 0, data, sizeof(data), 0, + 0); channel_queue.WaitForPreviouslyPostedTasks(); } @@ -144,8 +144,8 @@ TEST(ChannelSendFrameTransformerDelegateTest, [&callback](std::unique_ptr frame) { callback->OnTransformedFrame(CreateMockReceiverFrame()); }); - delegate->Transform(AudioFrameType::kEmptyFrame, 0, 0, 0, data, sizeof(data), - 0, 0); + delegate->Transform(AudioFrameType::kEmptyFrame, 0, 0, data, sizeof(data), 0, + 0); channel_queue.WaitForPreviouslyPostedTasks(); } diff --git a/audio/channel_send_unittest.cc b/audio/channel_send_unittest.cc index 137cd98efb..b9406e1523 100644 --- a/audio/channel_send_unittest.cc +++ b/audio/channel_send_unittest.cc @@ -19,7 +19,9 @@ #include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "call/rtp_transport_controller_send.h" +#include "rtc_base/gunit.h" #include "test/gtest.h" +#include "test/mock_frame_transformer.h" #include "test/mock_transport.h" #include "test/scoped_key_value_config.h" #include "test/time_controller/simulated_time_controller.h" @@ -31,6 +33,7 @@ namespace { using ::testing::Invoke; using ::testing::NiceMock; using ::testing::Return; +using ::testing::SaveArg; constexpr int kRtcpIntervalMs = 1000; constexpr int kSsrc = 333; @@ -145,6 +148,46 @@ TEST_F(ChannelSendTest, IncreaseRtpTimestampByPauseDuration) { EXPECT_EQ(timestamp_gap_ms, 10020); } +TEST_F(ChannelSendTest, FrameTransformerGetsCorrectTimestamp) { + rtc::scoped_refptr mock_frame_transformer = + rtc::make_ref_counted(); + channel_->SetEncoderToPacketizerFrameTransformer(mock_frame_transformer); + rtc::scoped_refptr callback; + EXPECT_CALL(*mock_frame_transformer, RegisterTransformedFrameCallback) + .WillOnce(SaveArg<0>(&callback)); + EXPECT_CALL(*mock_frame_transformer, UnregisterTransformedFrameCallback); + + absl::optional sent_timestamp; + auto send_rtp = [&](rtc::ArrayView data, + const PacketOptions& options) { + RtpPacketReceived packet; + packet.Parse(data); + if (!sent_timestamp) { + sent_timestamp = packet.Timestamp(); + } + return true; + }; + EXPECT_CALL(transport_, SendRtp).WillRepeatedly(Invoke(send_rtp)); + + channel_->StartSend(); + int64_t transformable_frame_timestamp = -1; + EXPECT_CALL(*mock_frame_transformer, Transform) + .WillOnce([&](std::unique_ptr frame) { + transformable_frame_timestamp = frame->GetTimestamp(); + callback->OnTransformedFrame(std::move(frame)); + }); + // Insert two frames which should trigger a new packet. + ProcessNextFrame(); + ProcessNextFrame(); + + // Ensure the RTP timestamp on the frame passed to the transformer + // includes the RTP offset and matches the actual RTP timestamp on the sent + // packet. + EXPECT_EQ_WAIT(transformable_frame_timestamp, + 0 + channel_->GetRtpRtcp()->StartTimestamp(), 1000); + EXPECT_TRUE_WAIT(sent_timestamp, 1000); + EXPECT_EQ(*sent_timestamp, transformable_frame_timestamp); +} } // namespace } // namespace voe } // namespace webrtc