From 7f60e5f753f8c739f2b8421eb341467452413bdf Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 23 Mar 2023 18:29:55 +0100 Subject: [PATCH] Cleanup IncludeCaptureClockOffset field trial Bug: webrtc:10739 Change-Id: I642cdf7574277c4c1b4ceb62b9e8a6905325dcfb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299004 Reviewed-by: Alessio Bazzica Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#39669} --- modules/rtp_rtcp/source/rtp_sender_audio.cc | 11 +--- modules/rtp_rtcp/source/rtp_sender_audio.h | 4 -- .../source/rtp_sender_audio_unittest.cc | 43 +----------- modules/rtp_rtcp/source/rtp_sender_video.cc | 11 +--- modules/rtp_rtcp/source/rtp_sender_video.h | 2 - .../source/rtp_sender_video_unittest.cc | 66 ++----------------- 6 files changed, 10 insertions(+), 127 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender_audio.cc b/modules/rtp_rtcp/source/rtp_sender_audio.cc index 244f644bd1..47b7b7910b 100644 --- a/modules/rtp_rtcp/source/rtp_sender_audio.cc +++ b/modules/rtp_rtcp/source/rtp_sender_audio.cc @@ -47,18 +47,12 @@ namespace { RTC_CHECK_NOTREACHED(); } -constexpr char kIncludeCaptureClockOffset[] = - "WebRTC-IncludeCaptureClockOffset"; - } // namespace RTPSenderAudio::RTPSenderAudio(Clock* clock, RTPSender* rtp_sender) : clock_(clock), rtp_sender_(rtp_sender), - absolute_capture_time_sender_(clock), - include_capture_clock_offset_( - !absl::StartsWith(field_trials_.Lookup(kIncludeCaptureClockOffset), - "Disabled")) { + absolute_capture_time_sender_(clock) { RTC_DCHECK(clock_); } @@ -284,8 +278,7 @@ bool RTPSenderAudio::SendAudio(AudioFrameType frame_type, encoder_rtp_timestamp_frequency.value_or(0), Int64MsToUQ32x32(clock_->ConvertTimestampToNtpTimeInMilliseconds( absolute_capture_timestamp_ms)), - /*estimated_capture_clock_offset=*/ - include_capture_clock_offset_ ? absl::make_optional(0) : absl::nullopt); + /*estimated_capture_clock_offset=*/0); if (absolute_capture_time) { // It also checks that extension was registered during SDP negotiation. If // not then setter won't do anything. diff --git a/modules/rtp_rtcp/source/rtp_sender_audio.h b/modules/rtp_rtcp/source/rtp_sender_audio.h index d40fee6386..e22ca41b90 100644 --- a/modules/rtp_rtcp/source/rtp_sender_audio.h +++ b/modules/rtp_rtcp/source/rtp_sender_audio.h @@ -17,7 +17,6 @@ #include #include "absl/strings/string_view.h" -#include "api/transport/field_trial_based_config.h" #include "modules/audio_coding/include/audio_coding_module_typedefs.h" #include "modules/rtp_rtcp/source/absolute_capture_time_sender.h" #include "modules/rtp_rtcp/source/dtmf_queue.h" @@ -112,9 +111,6 @@ class RTPSenderAudio { RTC_GUARDED_BY(send_audio_mutex_); AbsoluteCaptureTimeSender absolute_capture_time_sender_; - - const FieldTrialBasedConfig field_trials_; - const bool include_capture_clock_offset_; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_sender_audio_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_audio_unittest.cc index 7397a3ac4e..b497b5562c 100644 --- a/modules/rtp_rtcp/source/rtp_sender_audio_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_audio_unittest.cc @@ -13,14 +13,12 @@ #include #include -#include "api/transport/field_trial_based_config.h" #include "modules/rtp_rtcp/include/rtp_header_extension_map.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtp_header_extensions.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "modules/rtp_rtcp/source/rtp_rtcp_impl2.h" #include "rtc_base/thread.h" -#include "test/field_trial.h" #include "test/gmock.h" #include "test/gtest.h" @@ -150,41 +148,6 @@ TEST_F(RtpSenderAudioTest, SendAudioWithoutAbsoluteCaptureTime) { .HasExtension()); } -// Essentially the same test as -// SendAudioWithAbsoluteCaptureTimeWithCaptureClockOffset but with a field -// trial. We will remove this test eventually. -TEST_F(RtpSenderAudioTest, SendAudioWithAbsoluteCaptureTime) { - // Recreate rtp_sender_audio_ with new field trial. - test::ScopedFieldTrials field_trial( - "WebRTC-IncludeCaptureClockOffset/Disabled/"); - rtp_sender_audio_ = - std::make_unique(&fake_clock_, rtp_module_->RtpSender()); - - rtp_module_->RegisterRtpHeaderExtension(AbsoluteCaptureTimeExtension::Uri(), - kAbsoluteCaptureTimeExtensionId); - constexpr uint32_t kAbsoluteCaptureTimestampMs = 521; - const char payload_name[] = "audio"; - const uint8_t payload_type = 127; - ASSERT_EQ(0, rtp_sender_audio_->RegisterAudioPayload( - payload_name, payload_type, 48000, 0, 1500)); - uint8_t payload[] = {47, 11, 32, 93, 89}; - - ASSERT_TRUE(rtp_sender_audio_->SendAudio( - AudioFrameType::kAudioFrameCN, payload_type, 4321, payload, - sizeof(payload), kAbsoluteCaptureTimestampMs)); - - auto absolute_capture_time = - transport_.last_sent_packet() - .GetExtension(); - EXPECT_TRUE(absolute_capture_time); - EXPECT_EQ( - absolute_capture_time->absolute_capture_timestamp, - Int64MsToUQ32x32(fake_clock_.ConvertTimestampToNtpTimeInMilliseconds( - kAbsoluteCaptureTimestampMs))); - EXPECT_FALSE( - absolute_capture_time->estimated_capture_clock_offset.has_value()); -} - TEST_F(RtpSenderAudioTest, SendAudioWithAbsoluteCaptureTimeWithCaptureClockOffset) { rtp_module_->RegisterRtpHeaderExtension(AbsoluteCaptureTimeExtension::Uri(), @@ -203,14 +166,12 @@ TEST_F(RtpSenderAudioTest, auto absolute_capture_time = transport_.last_sent_packet() .GetExtension(); - EXPECT_TRUE(absolute_capture_time); + ASSERT_TRUE(absolute_capture_time); EXPECT_EQ( absolute_capture_time->absolute_capture_timestamp, Int64MsToUQ32x32(fake_clock_.ConvertTimestampToNtpTimeInMilliseconds( kAbsoluteCaptureTimestampMs))); - EXPECT_TRUE( - absolute_capture_time->estimated_capture_clock_offset.has_value()); - EXPECT_EQ(0, *absolute_capture_time->estimated_capture_clock_offset); + EXPECT_EQ(absolute_capture_time->estimated_capture_clock_offset, 0); } // As RFC4733, named telephone events are carried as part of the audio stream diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 83abbbca50..c863db4ccf 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -46,8 +46,6 @@ namespace webrtc { namespace { constexpr size_t kRedForFecHeaderLength = 1; constexpr int64_t kMaxUnretransmittableFrameIntervalMs = 33 * 4; -constexpr char kIncludeCaptureClockOffset[] = - "WebRTC-IncludeCaptureClockOffset"; void BuildRedPayload(const RtpPacketToSend& media_packet, RtpPacketToSend* red_packet) { @@ -172,10 +170,7 @@ RTPSenderVideo::RTPSenderVideo(const Config& config) rtp_sender_->SSRC(), rtp_sender_->Csrcs(), config.task_queue_factory) - : nullptr), - include_capture_clock_offset_(!absl::StartsWith( - config.field_trials->Lookup(kIncludeCaptureClockOffset), - "Disabled")) { + : nullptr) { if (frame_transformer_delegate_) frame_transformer_delegate_->Init(); } @@ -572,9 +567,7 @@ bool RTPSenderVideo::SendVideo( video_header.absolute_capture_time->absolute_capture_timestamp = Int64MsToUQ32x32( clock_->ConvertTimestampToNtpTime(*capture_time).ToMs()); - if (include_capture_clock_offset_) { - video_header.absolute_capture_time->estimated_capture_clock_offset = 0; - } + video_header.absolute_capture_time->estimated_capture_clock_offset = 0; } // Let `absolute_capture_time_sender_` decide if the extension should be sent. diff --git a/modules/rtp_rtcp/source/rtp_sender_video.h b/modules/rtp_rtcp/source/rtp_sender_video.h index d9474f3db5..a1388a8b6d 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.h +++ b/modules/rtp_rtcp/source/rtp_sender_video.h @@ -254,8 +254,6 @@ class RTPSenderVideo : public RTPVideoFrameSenderInterface { const rtc::scoped_refptr frame_transformer_delegate_; - - const bool include_capture_clock_offset_; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc index 25d6716302..683d082099 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc @@ -16,13 +16,11 @@ #include #include "absl/memory/memory.h" -#include "api/field_trials_registry.h" #include "api/frame_transformer_factory.h" #include "api/rtp_headers.h" #include "api/task_queue/task_queue_base.h" #include "api/task_queue/task_queue_factory.h" #include "api/test/mock_frame_encryptor.h" -#include "api/transport/field_trial_based_config.h" #include "api/transport/rtp/dependency_descriptor.h" #include "api/units/timestamp.h" #include "api/video/video_codec_constants.h" @@ -45,6 +43,7 @@ #include "rtc_base/logging.h" #include "rtc_base/rate_limiter.h" #include "rtc_base/thread.h" +#include "test/explicit_key_value_config.h" #include "test/gmock.h" #include "test/gtest.h" #include "test/mock_frame_transformer.h" @@ -156,25 +155,6 @@ class TestRtpSenderVideo : public RTPSenderVideo { } }; -class FieldTrials : public FieldTrialsRegistry { - public: - FieldTrials() : include_capture_clock_offset_(false) {} - - void set_include_capture_clock_offset(bool include_capture_clock_offset) { - include_capture_clock_offset_ = include_capture_clock_offset; - } - - private: - std::string GetValue(absl::string_view key) const override { - if (key == "WebRTC-IncludeCaptureClockOffset") { - return include_capture_clock_offset_ ? "" : "Disabled"; - } - return ""; - } - - bool include_capture_clock_offset_; -}; - class RtpSenderVideoTest : public ::testing::Test { public: RtpSenderVideoTest() @@ -205,7 +185,7 @@ class RtpSenderVideoTest : public ::testing::Test { protected: rtc::AutoThread main_thread_; const RtpRtcpInterface::Configuration config_; - FieldTrials field_trials_; + test::ExplicitKeyValueConfig field_trials_{""}; SimulatedClock fake_clock_; LoopbackTransportTest transport_; RateLimiter retransmission_rate_limiter_; @@ -1236,41 +1216,6 @@ TEST_F(RtpSenderVideoTest, VideoLayersAllocationNotSentOnHigherTemporalLayers) { .HasExtension()); } -TEST_F(RtpSenderVideoTest, AbsoluteCaptureTime) { - constexpr int64_t kAbsoluteCaptureTimestampMs = 12345678; - uint8_t kFrame[kMaxPacketLength]; - rtp_module_->RegisterRtpHeaderExtension(AbsoluteCaptureTimeExtension::Uri(), - kAbsoluteCaptureTimeExtensionId); - - RTPVideoHeader hdr; - hdr.frame_type = VideoFrameType::kVideoFrameKey; - rtp_sender_video_->SendVideo(kPayload, kType, kTimestamp, - kAbsoluteCaptureTimestampMs, kFrame, hdr, - kDefaultExpectedRetransmissionTimeMs, {}); - - absl::optional absolute_capture_time; - - // It is expected that one and only one of the packets sent on this video - // frame has absolute capture time header extension. - for (const RtpPacketReceived& packet : transport_.sent_packets()) { - if (absolute_capture_time.has_value()) { - EXPECT_FALSE(packet.HasExtension()); - } else { - absolute_capture_time = - packet.GetExtension(); - } - } - - // Verify the capture timestamp and that the clock offset is not set. - ASSERT_TRUE(absolute_capture_time.has_value()); - EXPECT_EQ( - absolute_capture_time->absolute_capture_timestamp, - Int64MsToUQ32x32(fake_clock_.ConvertTimestampToNtpTimeInMilliseconds( - kAbsoluteCaptureTimestampMs))); - EXPECT_FALSE( - absolute_capture_time->estimated_capture_clock_offset.has_value()); -} - TEST_F(RtpSenderVideoTest, AbsoluteCaptureTimeNotForwardedWhenImageHasNoCaptureTime) { uint8_t kFrame[kMaxPacketLength]; @@ -1289,10 +1234,7 @@ TEST_F(RtpSenderVideoTest, } } -// Essentially the same test as AbsoluteCaptureTime but with a field trial. -// After the field trial is experimented, we will remove AbsoluteCaptureTime. -TEST_F(RtpSenderVideoTest, AbsoluteCaptureTimeWithCaptureClockOffset) { - field_trials_.set_include_capture_clock_offset(true); +TEST_F(RtpSenderVideoTest, AbsoluteCaptureTime) { rtp_sender_video_ = std::make_unique( &fake_clock_, rtp_module_->RtpSender(), field_trials_); @@ -1499,7 +1441,7 @@ class RtpSenderVideoWithFrameTransformerTest : public ::testing::Test { protected: GlobalSimulatedTimeController time_controller_; - FieldTrialBasedConfig field_trials_; + test::ExplicitKeyValueConfig field_trials_{""}; LoopbackTransportTest transport_; RateLimiter retransmission_rate_limiter_; std::unique_ptr rtp_module_;