diff --git a/api/video/video_timing.cc b/api/video/video_timing.cc index fa14b9d5ae..d16911fb58 100644 --- a/api/video/video_timing.cc +++ b/api/video/video_timing.cc @@ -101,8 +101,8 @@ std::string TimingFrameInfo::ToString() const { } VideoPlayoutDelay::VideoPlayoutDelay(TimeDelta min, TimeDelta max) - : min_ms(std::clamp(min, TimeDelta::Zero(), kMax).ms()), - max_ms(std::clamp(max, min, kMax).ms()) { + : min_(std::clamp(min, TimeDelta::Zero(), kMax)), + max_(std::clamp(max, min_, kMax)) { if (!(TimeDelta::Zero() <= min && min <= max && max <= kMax)) { RTC_LOG(LS_ERROR) << "Invalid video playout delay: [" << min << "," << max << "]. Clamped to [" << this->min() << "," << this->max() @@ -112,8 +112,8 @@ VideoPlayoutDelay::VideoPlayoutDelay(TimeDelta min, TimeDelta max) bool VideoPlayoutDelay::Set(TimeDelta min, TimeDelta max) { if (TimeDelta::Zero() <= min && min <= max && max <= kMax) { - min_ms = min.ms(); - max_ms = max.ms(); + min_ = min; + max_ = max; return true; } return false; diff --git a/api/video/video_timing.h b/api/video/video_timing.h index 772e79549d..0a450cddaa 100644 --- a/api/video/video_timing.h +++ b/api/video/video_timing.h @@ -112,7 +112,9 @@ struct RTC_EXPORT TimingFrameInfo { // // min = x, max = y indicates that the receiver is free to adapt // in the range (x, y) based on network jitter. -struct RTC_EXPORT VideoPlayoutDelay { +// This class ensures invariant 0 <= min <= max <= kMax. +class RTC_EXPORT VideoPlayoutDelay { + public: // Maximum supported value for the delay limit. static constexpr TimeDelta kMax = TimeDelta::Millis(10) * 0xFFF; @@ -122,29 +124,25 @@ struct RTC_EXPORT VideoPlayoutDelay { return VideoPlayoutDelay(TimeDelta::Zero(), TimeDelta::Zero()); } + // Creates valid, but unspecified limits. VideoPlayoutDelay() = default; - VideoPlayoutDelay(int min_ms, int max_ms) : min_ms(min_ms), max_ms(max_ms) {} + VideoPlayoutDelay(const VideoPlayoutDelay&) = default; + VideoPlayoutDelay& operator=(const VideoPlayoutDelay&) = default; VideoPlayoutDelay(TimeDelta min, TimeDelta max); bool Set(TimeDelta min, TimeDelta max); - TimeDelta min() const { return TimeDelta::Millis(min_ms); } - TimeDelta max() const { return TimeDelta::Millis(max_ms); } + TimeDelta min() const { return min_; } + TimeDelta max() const { return max_; } - // TODO(bugs.webrtc.org/13756): Make members private and enforce the invariant - // in the constructors and setter. - bool Valid() const { - return TimeDelta::Zero() <= min() && min() <= max() && max() <= kMax; + friend bool operator==(const VideoPlayoutDelay& lhs, + const VideoPlayoutDelay& rhs) { + return lhs.min_ == rhs.min_ && lhs.max_ == rhs.max_; } - // TODO(bugs.webrtc.org/13756): Make members private and change their type to - // TimeDelta. - int min_ms = -1; - int max_ms = -1; - - bool operator==(const VideoPlayoutDelay& rhs) const { - return min_ms == rhs.min_ms && max_ms == rhs.max_ms; - } + private: + TimeDelta min_ = TimeDelta::Zero(); + TimeDelta max_ = kMax; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_header_extensions.cc b/modules/rtp_rtcp/source/rtp_header_extensions.cc index 4745b10e54..e42a84bd06 100644 --- a/modules/rtp_rtcp/source/rtp_header_extensions.cc +++ b/modules/rtp_rtcp/source/rtp_header_extensions.cc @@ -400,22 +400,24 @@ bool PlayoutDelayLimits::Parse(rtc::ArrayView data, uint32_t raw = ByteReader::ReadBigEndian(data.data()); uint16_t min_raw = (raw >> 12); uint16_t max_raw = (raw & 0xfff); - if (min_raw > max_raw) - return false; - playout_delay->min_ms = min_raw * kGranularityMs; - playout_delay->max_ms = max_raw * kGranularityMs; - return true; + return playout_delay->Set(min_raw * kGranularity, max_raw * kGranularity); } bool PlayoutDelayLimits::Write(rtc::ArrayView data, const VideoPlayoutDelay& playout_delay) { RTC_DCHECK_EQ(data.size(), 3); - RTC_DCHECK_LE(0, playout_delay.min_ms); - RTC_DCHECK_LE(playout_delay.min_ms, playout_delay.max_ms); - RTC_DCHECK_LE(playout_delay.max_ms, kMaxMs); - // Convert MS to value to be sent on extension header. - uint32_t min_delay = playout_delay.min_ms / kGranularityMs; - uint32_t max_delay = playout_delay.max_ms / kGranularityMs; + + // Convert TimeDelta to value to be sent on extension header. + auto idiv = [](TimeDelta num, TimeDelta den) { return num.us() / den.us(); }; + int64_t min_delay = idiv(playout_delay.min(), kGranularity); + int64_t max_delay = idiv(playout_delay.max(), kGranularity); + + // Double check min/max boundaries guaranteed by the `VideoPlayouDelay` type. + RTC_DCHECK_GE(min_delay, 0); + RTC_DCHECK_LT(min_delay, 1 << 12); + RTC_DCHECK_GE(max_delay, 0); + RTC_DCHECK_LT(max_delay, 1 << 12); + ByteWriter::WriteBigEndian(data.data(), (min_delay << 12) | max_delay); return true; diff --git a/modules/rtp_rtcp/source/rtp_header_extensions.h b/modules/rtp_rtcp/source/rtp_header_extensions.h index 04b2cd63a6..739d4765d0 100644 --- a/modules/rtp_rtcp/source/rtp_header_extensions.h +++ b/modules/rtp_rtcp/source/rtp_header_extensions.h @@ -197,9 +197,9 @@ class PlayoutDelayLimits { // Playout delay in milliseconds. A playout delay limit (min or max) // has 12 bits allocated. This allows a range of 0-4095 values which // translates to a range of 0-40950 in milliseconds. - static constexpr int kGranularityMs = 10; + static constexpr TimeDelta kGranularity = TimeDelta::Millis(10); // Maximum playout delay value in milliseconds. - static constexpr int kMaxMs = 0xfff * kGranularityMs; // 40950. + static constexpr TimeDelta kMax = 0xfff * kGranularity; // 40950. static bool Parse(rtc::ArrayView data, VideoPlayoutDelay* playout_delay); diff --git a/modules/rtp_rtcp/source/rtp_packet_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_unittest.cc index bf9f757647..a1d1c9d4df 100644 --- a/modules/rtp_rtcp/source/rtp_packet_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_unittest.cc @@ -253,8 +253,9 @@ TEST(RtpPacketTest, CreateWithTwoByteHeaderExtensionFirst) { packet.SetTimestamp(kTimestamp); packet.SetSsrc(kSsrc); // Set extension that requires two-byte header. - VideoPlayoutDelay playoutDelay = {30, 340}; - ASSERT_TRUE(packet.SetExtension(playoutDelay)); + VideoPlayoutDelay playout_delay(TimeDelta::Millis(30), + TimeDelta::Millis(340)); + ASSERT_TRUE(packet.SetExtension(playout_delay)); packet.SetExtension(kTimeOffset); packet.SetExtension(kVoiceActive, kAudioLevel); EXPECT_THAT(kPacketWithTwoByteExtensionIdFirst, @@ -277,8 +278,9 @@ TEST(RtpPacketTest, CreateWithTwoByteHeaderExtensionLast) { EXPECT_THAT(kPacketWithTOAndAL, ElementsAreArray(packet.data(), packet.size())); // Set extension that requires two-byte header. - VideoPlayoutDelay playoutDelay = {30, 340}; - ASSERT_TRUE(packet.SetExtension(playoutDelay)); + VideoPlayoutDelay playout_delay(TimeDelta::Millis(30), + TimeDelta::Millis(340)); + ASSERT_TRUE(packet.SetExtension(playout_delay)); EXPECT_THAT(kPacketWithTwoByteExtensionIdLast, ElementsAreArray(packet.data(), packet.size())); } diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index ca43f1d324..b846f7f7ea 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -106,8 +106,9 @@ absl::optional LoadVideoPlayoutDelayOverride( ParseFieldTrial({&playout_delay_max_ms, &playout_delay_min_ms}, key_value_config->Lookup("WebRTC-ForceSendPlayoutDelay")); return playout_delay_max_ms && playout_delay_min_ms - ? absl::make_optional(*playout_delay_min_ms, - *playout_delay_max_ms) + ? absl::make_optional( + TimeDelta::Millis(*playout_delay_min_ms), + TimeDelta::Millis(*playout_delay_max_ms)) : absl::nullopt; } @@ -862,11 +863,6 @@ void RTPSenderVideo::MaybeUpdateCurrentPlayoutDelay( return; } - if (!requested_delay->Valid()) { - RTC_DLOG(LS_ERROR) << "Requested playout delay values out of order"; - return; - } - current_playout_delay_ = requested_delay; playout_delay_pending_ = true; } diff --git a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc index 5b7be7ed0e..9641d617d9 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc @@ -1333,7 +1333,8 @@ TEST_F(RtpSenderVideoTest, PopulatesPlayoutDelay) { uint8_t kFrame[kPacketSize]; rtp_module_->RegisterRtpHeaderExtension(PlayoutDelayLimits::Uri(), kPlayoutDelayExtensionId); - const VideoPlayoutDelay kExpectedDelay = {10, 20}; + const VideoPlayoutDelay kExpectedDelay(TimeDelta::Millis(10), + TimeDelta::Millis(20)); // Send initial key-frame without playout delay. RTPVideoHeader hdr; @@ -1362,7 +1363,7 @@ TEST_F(RtpSenderVideoTest, PopulatesPlayoutDelay) { // Set playout delay on a non-discardable frame, the extension should still // be populated since dilvery wasn't guaranteed on the last one. - hdr.playout_delay = VideoPlayoutDelay(); // Indicates "no change". + hdr.playout_delay = absl::nullopt; // Indicates "no change". vp8_header.temporalIdx = 0; rtp_sender_video_->SendVideo( kPayload, kType, kTimestamp, fake_clock_.CurrentTime(), kFrame, diff --git a/modules/video_coding/generic_decoder_unittest.cc b/modules/video_coding/generic_decoder_unittest.cc index 529f13fdb8..32ae4bf42d 100644 --- a/modules/video_coding/generic_decoder_unittest.cc +++ b/modules/video_coding/generic_decoder_unittest.cc @@ -177,9 +177,10 @@ TEST_F(GenericDecoderTest, IsLowLatencyStreamFalseByDefault) { TEST_F(GenericDecoderTest, IsLowLatencyStreamActivatedByPlayoutDelay) { VCMEncodedFrame encoded_frame; - const VideoPlayoutDelay kPlayoutDelay = {0, 50}; - timing_.set_min_playout_delay(TimeDelta::Millis(kPlayoutDelay.min_ms)); - timing_.set_max_playout_delay(TimeDelta::Millis(kPlayoutDelay.max_ms)); + const VideoPlayoutDelay kPlayoutDelay(TimeDelta::Zero(), + TimeDelta::Millis(50)); + timing_.set_min_playout_delay(kPlayoutDelay.min()); + timing_.set_max_playout_delay(kPlayoutDelay.max()); generic_decoder_.Decode(encoded_frame, clock_->CurrentTime()); time_controller_.AdvanceTime(TimeDelta::Millis(10)); absl::optional decoded_frame = user_callback_.PopLastFrame(); diff --git a/video/rtp_video_stream_receiver2_unittest.cc b/video/rtp_video_stream_receiver2_unittest.cc index 9085863922..d82f7bb9a5 100644 --- a/video/rtp_video_stream_receiver2_unittest.cc +++ b/video/rtp_video_stream_receiver2_unittest.cc @@ -1174,17 +1174,23 @@ TEST_F(RtpVideoStreamReceiver2Test, TransformFrame) { } // Test default behavior and when playout delay is overridden by field trial. -const VideoPlayoutDelay kTransmittedPlayoutDelay = {100, 200}; -const VideoPlayoutDelay kForcedPlayoutDelay = {70, 90}; +VideoPlayoutDelay TransmittedPlayoutDelay() { + return {TimeDelta::Millis(100), TimeDelta::Millis(200)}; +} +VideoPlayoutDelay ForcedPlayoutDelay() { + return {TimeDelta::Millis(70), TimeDelta::Millis(90)}; +} struct PlayoutDelayOptions { std::string field_trial; VideoPlayoutDelay expected_delay; }; -const PlayoutDelayOptions kDefaultBehavior = { - /*field_trial=*/"", /*expected_delay=*/kTransmittedPlayoutDelay}; -const PlayoutDelayOptions kOverridePlayoutDelay = { - /*field_trial=*/"WebRTC-ForcePlayoutDelay/min_ms:70,max_ms:90/", - /*expected_delay=*/kForcedPlayoutDelay}; +PlayoutDelayOptions DefaultBehavior() { + return {.field_trial = "", .expected_delay = TransmittedPlayoutDelay()}; +} +PlayoutDelayOptions OverridePlayoutDelay() { + return {.field_trial = "WebRTC-ForcePlayoutDelay/min_ms:70,max_ms:90/", + .expected_delay = ForcedPlayoutDelay()}; +} class RtpVideoStreamReceiver2TestPlayoutDelay : public RtpVideoStreamReceiver2Test, @@ -1196,7 +1202,7 @@ class RtpVideoStreamReceiver2TestPlayoutDelay INSTANTIATE_TEST_SUITE_P(PlayoutDelay, RtpVideoStreamReceiver2TestPlayoutDelay, - Values(kDefaultBehavior, kOverridePlayoutDelay)); + Values(DefaultBehavior(), OverridePlayoutDelay())); TEST_P(RtpVideoStreamReceiver2TestPlayoutDelay, PlayoutDelay) { rtc::CopyOnWriteBuffer payload_data({'1', '2', '3', '4'}); @@ -1208,7 +1214,7 @@ TEST_P(RtpVideoStreamReceiver2TestPlayoutDelay, PlayoutDelay) { // Set playout delay on outgoing packet. EXPECT_TRUE(packet_to_send.SetExtension( - kTransmittedPlayoutDelay)); + TransmittedPlayoutDelay())); uint8_t* payload = packet_to_send.AllocatePayload(payload_data.size()); memcpy(payload, payload_data.data(), payload_data.size()); diff --git a/video/video_receive_stream2_unittest.cc b/video/video_receive_stream2_unittest.cc index c4388f7a50..4476c46127 100644 --- a/video/video_receive_stream2_unittest.cc +++ b/video/video_receive_stream2_unittest.cc @@ -314,18 +314,19 @@ TEST_P(VideoReceiveStream2Test, CreateFrameFromH264FmtpSpropAndIdr) { } TEST_P(VideoReceiveStream2Test, PlayoutDelay) { - const VideoPlayoutDelay kPlayoutDelayMs = {123, 321}; + const VideoPlayoutDelay kPlayoutDelay(TimeDelta::Millis(123), + TimeDelta::Millis(321)); std::unique_ptr test_frame = test::FakeFrameBuilder() .Id(0) - .PlayoutDelay(kPlayoutDelayMs) + .PlayoutDelay(kPlayoutDelay) .AsLast() .Build(); video_receive_stream_->OnCompleteFrame(std::move(test_frame)); auto timings = timing_->GetTimings(); - EXPECT_EQ(kPlayoutDelayMs.min_ms, timings.min_playout_delay.ms()); - EXPECT_EQ(kPlayoutDelayMs.max_ms, timings.max_playout_delay.ms()); + EXPECT_EQ(kPlayoutDelay.min(), timings.min_playout_delay); + EXPECT_EQ(kPlayoutDelay.max(), timings.max_playout_delay); // Check that the biggest minimum delay is chosen. video_receive_stream_->SetMinimumPlayoutDelay(400); @@ -373,7 +374,7 @@ TEST_P(VideoReceiveStream2Test, UseLowLatencyRenderingSetFromPlayoutDelay) { std::unique_ptr test_frame1 = test::FakeFrameBuilder() .Id(1) - .PlayoutDelay({/*min_ms=*/0, /*max_ms=*/500}) + .PlayoutDelay({TimeDelta::Zero(), TimeDelta::Millis(500)}) .AsLast() .Build(); video_receive_stream_->OnCompleteFrame(std::move(test_frame1)); @@ -404,7 +405,7 @@ TEST_P(VideoReceiveStream2Test, MaxCompositionDelaySetFromMaxPlayoutDelay) { .Id(1) .Time(RtpTimestampForFrame(1)) .ReceivedTime(ReceiveTimeForFrame(1)) - .PlayoutDelay({0, 0}) + .PlayoutDelay(VideoPlayoutDelay::Minimal()) .AsLast() .Build(); video_receive_stream_->OnCompleteFrame(std::move(test_frame1)); @@ -418,7 +419,7 @@ TEST_P(VideoReceiveStream2Test, MaxCompositionDelaySetFromMaxPlayoutDelay) { .Id(2) .Time(RtpTimestampForFrame(2)) .ReceivedTime(ReceiveTimeForFrame(2)) - .PlayoutDelay({10, 30}) + .PlayoutDelay({TimeDelta::Millis(10), TimeDelta::Millis(30)}) .AsLast() .Build(); video_receive_stream_->OnCompleteFrame(std::move(test_frame2)); @@ -434,7 +435,7 @@ TEST_P(VideoReceiveStream2Test, MaxCompositionDelaySetFromMaxPlayoutDelay) { .Id(3) .Time(RtpTimestampForFrame(3)) .ReceivedTime(ReceiveTimeForFrame(3)) - .PlayoutDelay({0, 50}) + .PlayoutDelay({TimeDelta::Zero(), TimeDelta::Millis(50)}) .AsLast() .Build(); video_receive_stream_->OnCompleteFrame(std::move(test_frame3)); diff --git a/video/video_stream_buffer_controller_unittest.cc b/video/video_stream_buffer_controller_unittest.cc index a3b766da80..3224b20d83 100644 --- a/video/video_stream_buffer_controller_unittest.cc +++ b/video/video_stream_buffer_controller_unittest.cc @@ -827,7 +827,7 @@ TEST_P(LowLatencyVideoStreamBufferControllerTest, auto frame = test::FakeFrameBuilder() .Id(0) .Time(0) - .PlayoutDelay({0, 10}) + .PlayoutDelay({TimeDelta::Zero(), TimeDelta::Millis(10)}) .AsLast() .Build(); buffer_->InsertFrame(std::move(frame)); @@ -839,7 +839,7 @@ TEST_P(LowLatencyVideoStreamBufferControllerTest, frame = test::FakeFrameBuilder() .Id(1) .Time(kFps30Rtp) - .PlayoutDelay({0, 10}) + .PlayoutDelay({TimeDelta::Zero(), TimeDelta::Millis(10)}) .AsLast() .Build(); buffer_->InsertFrame(std::move(frame)); @@ -857,7 +857,7 @@ TEST_P(LowLatencyVideoStreamBufferControllerTest, ZeroPlayoutDelayFullQueue) { auto frame = test::FakeFrameBuilder() .Id(0) .Time(0) - .PlayoutDelay({0, 10}) + .PlayoutDelay({TimeDelta::Zero(), TimeDelta::Millis(10)}) .AsLast() .Build(); // Playout delay of 0 implies low-latency rendering. @@ -869,7 +869,7 @@ TEST_P(LowLatencyVideoStreamBufferControllerTest, ZeroPlayoutDelayFullQueue) { frame = test::FakeFrameBuilder() .Id(id) .Time(kFps30Rtp * id) - .PlayoutDelay({0, 10}) + .PlayoutDelay({TimeDelta::Zero(), TimeDelta::Millis(10)}) .AsLast() .Build(); buffer_->InsertFrame(std::move(frame));