From f53597140f2485871f0e54b26c3d5b0feb5bcc14 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 28 Aug 2023 16:32:25 +0000 Subject: [PATCH] In RtpSource represent time with Timestamp type instead of int64_t Bug: webrtc:13757 Change-Id: I5d7da9c9aee489e4b57d361de174c59713cb2b14 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/317780 Reviewed-by: Harald Alvestrand Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#40650} --- api/transport/rtp/BUILD.gn | 1 + api/transport/rtp/rtp_source.h | 32 ++++- media/engine/webrtc_video_engine_unittest.cc | 6 +- modules/rtp_rtcp/source/source_tracker.cc | 2 +- .../source/source_tracker_unittest.cc | 130 +++++++++--------- video/video_receive_stream2_unittest.cc | 12 +- 6 files changed, 98 insertions(+), 85 deletions(-) diff --git a/api/transport/rtp/BUILD.gn b/api/transport/rtp/BUILD.gn index 205bbcc988..6f2a15e0e9 100644 --- a/api/transport/rtp/BUILD.gn +++ b/api/transport/rtp/BUILD.gn @@ -14,6 +14,7 @@ rtc_source_set("rtp_source") { deps = [ "../../../api:rtp_headers", "../../../api/units:time_delta", + "../../../api/units:timestamp", "../../../rtc_base:checks", ] absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] diff --git a/api/transport/rtp/rtp_source.h b/api/transport/rtp/rtp_source.h index e51dcd70b6..41a0552db0 100644 --- a/api/transport/rtp/rtp_source.h +++ b/api/transport/rtp/rtp_source.h @@ -16,6 +16,7 @@ #include "absl/types/optional.h" #include "api/rtp_headers.h" #include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "rtc_base/checks.h" namespace webrtc { @@ -44,12 +45,25 @@ class RtpSource { RtpSource() = delete; + RtpSource(Timestamp timestamp, + uint32_t source_id, + RtpSourceType source_type, + uint32_t rtp_timestamp, + const RtpSource::Extensions& extensions) + : timestamp_(timestamp), + source_id_(source_id), + source_type_(source_type), + extensions_(extensions), + rtp_timestamp_(rtp_timestamp) {} + + // TODO(bugs.webrtc.org/13757): deprecate when chromium stop using this + // and remove after 2023-09-18 RtpSource(int64_t timestamp_ms, uint32_t source_id, RtpSourceType source_type, uint32_t rtp_timestamp, const RtpSource::Extensions& extensions) - : timestamp_ms_(timestamp_ms), + : timestamp_(Timestamp::Millis(timestamp_ms)), source_id_(source_id), source_type_(source_type), extensions_(extensions), @@ -59,10 +73,14 @@ class RtpSource { RtpSource& operator=(const RtpSource&) = default; ~RtpSource() = default; - int64_t timestamp_ms() const { return timestamp_ms_; } - void update_timestamp_ms(int64_t timestamp_ms) { - RTC_DCHECK_LE(timestamp_ms_, timestamp_ms); - timestamp_ms_ = timestamp_ms; + Timestamp timestamp() const { return timestamp_; } + + // TODO(bugs.webrtc.org/13757): deprecate when chromium stop using this + // and remove after 2023-09-18 + int64_t timestamp_ms() const { return timestamp_.ms(); } + [[deprecated]] void update_timestamp_ms(int64_t timestamp_ms) { + RTC_DCHECK_LE(timestamp_.ms(), timestamp_ms); + timestamp_ = Timestamp::Millis(timestamp_ms); } // The identifier of the source can be the CSRC or the SSRC. @@ -90,7 +108,7 @@ class RtpSource { } bool operator==(const RtpSource& o) const { - return timestamp_ms_ == o.timestamp_ms() && source_id_ == o.source_id() && + return timestamp_ == o.timestamp() && source_id_ == o.source_id() && source_type_ == o.source_type() && extensions_.audio_level == o.extensions_.audio_level && extensions_.absolute_capture_time == @@ -99,7 +117,7 @@ class RtpSource { } private: - int64_t timestamp_ms_; + Timestamp timestamp_; uint32_t source_id_; RtpSourceType source_type_; RtpSource::Extensions extensions_; diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 7719dc293e..887588341f 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -9767,7 +9767,7 @@ TEST_F(WebRtcVideoChannelBaseTest, GetSources) { EXPECT_EQ(source.source_id(), kSsrc); EXPECT_EQ(source.source_type(), webrtc::RtpSourceType::SSRC); int64_t rtp_timestamp_1 = source.rtp_timestamp(); - int64_t timestamp_ms_1 = source.timestamp_ms(); + Timestamp timestamp_1 = source.timestamp(); // Send and receive another frame. SendFrame(); @@ -9781,10 +9781,10 @@ TEST_F(WebRtcVideoChannelBaseTest, GetSources) { EXPECT_EQ(source.source_id(), kSsrc); EXPECT_EQ(source.source_type(), webrtc::RtpSourceType::SSRC); int64_t rtp_timestamp_2 = source.rtp_timestamp(); - int64_t timestamp_ms_2 = source.timestamp_ms(); + Timestamp timestamp_2 = source.timestamp(); EXPECT_GT(rtp_timestamp_2, rtp_timestamp_1); - EXPECT_GT(timestamp_ms_2, timestamp_ms_1); + EXPECT_GT(timestamp_2, timestamp_1); } TEST_F(WebRtcVideoChannelTest, SetsRidsOnSendStream) { diff --git a/modules/rtp_rtcp/source/source_tracker.cc b/modules/rtp_rtcp/source/source_tracker.cc index 13d848dce0..51e8f1cd38 100644 --- a/modules/rtp_rtcp/source/source_tracker.cc +++ b/modules/rtp_rtcp/source/source_tracker.cc @@ -79,7 +79,7 @@ std::vector SourceTracker::GetSources() const { const SourceEntry& entry = pair.second; sources.emplace_back( - entry.timestamp.ms(), key.source, key.source_type, entry.rtp_timestamp, + entry.timestamp, key.source, key.source_type, entry.rtp_timestamp, RtpSource::Extensions{ .audio_level = entry.audio_level, .absolute_capture_time = entry.absolute_capture_time, diff --git a/modules/rtp_rtcp/source/source_tracker_unittest.cc b/modules/rtp_rtcp/source/source_tracker_unittest.cc index c11142bcb4..e14a389534 100644 --- a/modules/rtp_rtcp/source/source_tracker_unittest.cc +++ b/modules/rtp_rtcp/source/source_tracker_unittest.cc @@ -46,7 +46,7 @@ class ExpectedSourceTracker { explicit ExpectedSourceTracker(Clock* clock) : clock_(clock) {} void OnFrameDelivered(const RtpPacketInfos& packet_infos) { - const int64_t now_ms = clock_->TimeInMilliseconds(); + const Timestamp now = clock_->CurrentTime(); for (const auto& packet_info : packet_infos) { RtpSource::Extensions extensions = { @@ -54,26 +54,26 @@ class ExpectedSourceTracker { packet_info.local_capture_clock_offset()}; for (const auto& csrc : packet_info.csrcs()) { - entries_.emplace_front(now_ms, csrc, RtpSourceType::CSRC, + entries_.emplace_front(now, csrc, RtpSourceType::CSRC, packet_info.rtp_timestamp(), extensions); } - entries_.emplace_front(now_ms, packet_info.ssrc(), RtpSourceType::SSRC, + entries_.emplace_front(now, packet_info.ssrc(), RtpSourceType::SSRC, packet_info.rtp_timestamp(), extensions); } - PruneEntries(now_ms); + PruneEntries(now); } std::vector GetSources() const { - PruneEntries(clock_->TimeInMilliseconds()); + PruneEntries(clock_->CurrentTime()); return std::vector(entries_.begin(), entries_.end()); } private: - void PruneEntries(int64_t now_ms) const { - const int64_t prune_ms = now_ms - 10000; // 10 seconds + void PruneEntries(Timestamp now) const { + const Timestamp prune = now - TimeDelta::Seconds(10); std::set> seen; @@ -84,7 +84,7 @@ class ExpectedSourceTracker { ++next; auto key = std::make_pair(it->source_type(), it->source_id()); - if (!seen.insert(key).second || it->timestamp_ms() < prune_ms) { + if (!seen.insert(key).second || it->timestamp() < prune) { entries_.erase(it); } @@ -297,18 +297,17 @@ TEST(SourceTrackerTest, OnFrameDeliveredRecordsSourcesDistinctSsrcs) { time_controller.AdvanceTime(TimeDelta::Zero()); - EXPECT_THAT( - tracker.GetSources(), - ElementsAre(RtpSource(timestamp.ms(), kSsrc2, RtpSourceType::SSRC, - kRtpTimestamp1, extensions1), - RtpSource(timestamp.ms(), kCsrcs2, RtpSourceType::CSRC, - kRtpTimestamp1, extensions1), - RtpSource(timestamp.ms(), kSsrc1, RtpSourceType::SSRC, - kRtpTimestamp0, extensions0), - RtpSource(timestamp.ms(), kCsrcs1, RtpSourceType::CSRC, - kRtpTimestamp0, extensions0), - RtpSource(timestamp.ms(), kCsrcs0, RtpSourceType::CSRC, - kRtpTimestamp0, extensions0))); + EXPECT_THAT(tracker.GetSources(), + ElementsAre(RtpSource(timestamp, kSsrc2, RtpSourceType::SSRC, + kRtpTimestamp1, extensions1), + RtpSource(timestamp, kCsrcs2, RtpSourceType::CSRC, + kRtpTimestamp1, extensions1), + RtpSource(timestamp, kSsrc1, RtpSourceType::SSRC, + kRtpTimestamp0, extensions0), + RtpSource(timestamp, kCsrcs1, RtpSourceType::CSRC, + kRtpTimestamp0, extensions0), + RtpSource(timestamp, kCsrcs0, RtpSourceType::CSRC, + kRtpTimestamp0, extensions0))); } TEST(SourceTrackerTest, OnFrameDeliveredRecordsSourcesSameSsrc) { @@ -363,16 +362,15 @@ TEST(SourceTrackerTest, OnFrameDeliveredRecordsSourcesSameSsrc) { .absolute_capture_time = kAbsoluteCaptureTime, .local_capture_clock_offset = kLocalCaptureClockOffset}; - EXPECT_THAT( - tracker.GetSources(), - ElementsAre(RtpSource(timestamp.ms(), kSsrc, RtpSourceType::SSRC, - kRtpTimestamp2, extensions2), - RtpSource(timestamp.ms(), kCsrcs0, RtpSourceType::CSRC, - kRtpTimestamp2, extensions2), - RtpSource(timestamp.ms(), kCsrcs2, RtpSourceType::CSRC, - kRtpTimestamp1, extensions1), - RtpSource(timestamp.ms(), kCsrcs1, RtpSourceType::CSRC, - kRtpTimestamp0, extensions0))); + EXPECT_THAT(tracker.GetSources(), + ElementsAre(RtpSource(timestamp, kSsrc, RtpSourceType::SSRC, + kRtpTimestamp2, extensions2), + RtpSource(timestamp, kCsrcs0, RtpSourceType::CSRC, + kRtpTimestamp2, extensions2), + RtpSource(timestamp, kCsrcs2, RtpSourceType::CSRC, + kRtpTimestamp1, extensions1), + RtpSource(timestamp, kCsrcs1, RtpSourceType::CSRC, + kRtpTimestamp0, extensions0))); } TEST(SourceTrackerTest, OnFrameDeliveredUpdatesSources) { @@ -427,14 +425,13 @@ TEST(SourceTrackerTest, OnFrameDeliveredUpdatesSources) { time_controller.AdvanceTime(TimeDelta::Zero()); Timestamp timestamp_0 = time_controller.GetClock()->CurrentTime(); - EXPECT_THAT( - tracker.GetSources(), - ElementsAre(RtpSource(timestamp_0.ms(), kSsrc1, RtpSourceType::SSRC, - kRtpTimestamp0, extensions0), - RtpSource(timestamp_0.ms(), kCsrcs1, RtpSourceType::CSRC, - kRtpTimestamp0, extensions0), - RtpSource(timestamp_0.ms(), kCsrcs0, RtpSourceType::CSRC, - kRtpTimestamp0, extensions0))); + EXPECT_THAT(tracker.GetSources(), + ElementsAre(RtpSource(timestamp_0, kSsrc1, RtpSourceType::SSRC, + kRtpTimestamp0, extensions0), + RtpSource(timestamp_0, kCsrcs1, RtpSourceType::CSRC, + kRtpTimestamp0, extensions0), + RtpSource(timestamp_0, kCsrcs0, RtpSourceType::CSRC, + kRtpTimestamp0, extensions0))); // Deliver packets with updated sources. @@ -448,16 +445,15 @@ TEST(SourceTrackerTest, OnFrameDeliveredUpdatesSources) { time_controller.AdvanceTime(TimeDelta::Zero()); Timestamp timestamp_1 = time_controller.GetClock()->CurrentTime(); - EXPECT_THAT( - tracker.GetSources(), - ElementsAre(RtpSource(timestamp_1.ms(), kSsrc1, RtpSourceType::SSRC, - kRtpTimestamp1, extensions1), - RtpSource(timestamp_1.ms(), kCsrcs2, RtpSourceType::CSRC, - kRtpTimestamp1, extensions1), - RtpSource(timestamp_1.ms(), kCsrcs0, RtpSourceType::CSRC, - kRtpTimestamp1, extensions1), - RtpSource(timestamp_0.ms(), kCsrcs1, RtpSourceType::CSRC, - kRtpTimestamp0, extensions0))); + EXPECT_THAT(tracker.GetSources(), + ElementsAre(RtpSource(timestamp_1, kSsrc1, RtpSourceType::SSRC, + kRtpTimestamp1, extensions1), + RtpSource(timestamp_1, kCsrcs2, RtpSourceType::CSRC, + kRtpTimestamp1, extensions1), + RtpSource(timestamp_1, kCsrcs0, RtpSourceType::CSRC, + kRtpTimestamp1, extensions1), + RtpSource(timestamp_0, kCsrcs1, RtpSourceType::CSRC, + kRtpTimestamp0, extensions0))); // Deliver more packets with update csrcs and a new ssrc. time_controller.AdvanceTime(TimeDelta::Millis(17)); @@ -471,18 +467,17 @@ TEST(SourceTrackerTest, OnFrameDeliveredUpdatesSources) { time_controller.AdvanceTime(TimeDelta::Zero()); Timestamp timestamp_2 = time_controller.GetClock()->CurrentTime(); - EXPECT_THAT( - tracker.GetSources(), - ElementsAre(RtpSource(timestamp_2.ms(), kSsrc2, RtpSourceType::SSRC, - kRtpTimestamp2, extensions2), - RtpSource(timestamp_2.ms(), kCsrcs0, RtpSourceType::CSRC, - kRtpTimestamp2, extensions2), - RtpSource(timestamp_1.ms(), kSsrc1, RtpSourceType::SSRC, - kRtpTimestamp1, extensions1), - RtpSource(timestamp_1.ms(), kCsrcs2, RtpSourceType::CSRC, - kRtpTimestamp1, extensions1), - RtpSource(timestamp_0.ms(), kCsrcs1, RtpSourceType::CSRC, - kRtpTimestamp0, extensions0))); + EXPECT_THAT(tracker.GetSources(), + ElementsAre(RtpSource(timestamp_2, kSsrc2, RtpSourceType::SSRC, + kRtpTimestamp2, extensions2), + RtpSource(timestamp_2, kCsrcs0, RtpSourceType::CSRC, + kRtpTimestamp2, extensions2), + RtpSource(timestamp_1, kSsrc1, RtpSourceType::SSRC, + kRtpTimestamp1, extensions1), + RtpSource(timestamp_1, kCsrcs2, RtpSourceType::CSRC, + kRtpTimestamp1, extensions1), + RtpSource(timestamp_0, kCsrcs1, RtpSourceType::CSRC, + kRtpTimestamp0, extensions0))); } TEST(SourceTrackerTest, TimedOutSourcesAreRemoved) { @@ -531,14 +526,13 @@ TEST(SourceTrackerTest, TimedOutSourcesAreRemoved) { .absolute_capture_time = kAbsoluteCaptureTime1, .local_capture_clock_offset = kLocalCaptureClockOffset1}; - EXPECT_THAT( - tracker.GetSources(), - ElementsAre(RtpSource(timestamp_1.ms(), kSsrc, RtpSourceType::SSRC, - kRtpTimestamp1, extensions1), - RtpSource(timestamp_1.ms(), kCsrcs2, RtpSourceType::CSRC, - kRtpTimestamp1, extensions1), - RtpSource(timestamp_1.ms(), kCsrcs0, RtpSourceType::CSRC, - kRtpTimestamp1, extensions1))); + EXPECT_THAT(tracker.GetSources(), + ElementsAre(RtpSource(timestamp_1, kSsrc, RtpSourceType::SSRC, + kRtpTimestamp1, extensions1), + RtpSource(timestamp_1, kCsrcs2, RtpSourceType::CSRC, + kRtpTimestamp1, extensions1), + RtpSource(timestamp_1, kCsrcs0, RtpSourceType::CSRC, + kRtpTimestamp1, extensions1))); } } // namespace webrtc diff --git a/video/video_receive_stream2_unittest.cc b/video/video_receive_stream2_unittest.cc index 4476c46127..7ae446fc16 100644 --- a/video/video_receive_stream2_unittest.cc +++ b/video/video_receive_stream2_unittest.cc @@ -563,12 +563,12 @@ TEST_P(VideoReceiveStream2Test, RenderedFrameUpdatesGetSources) { EXPECT_THAT(video_receive_stream_->GetSources(), IsEmpty()); // Render one video frame. - int64_t timestamp_ms_min = clock_->TimeInMilliseconds(); + Timestamp timestamp_min = clock_->CurrentTime(); video_receive_stream_->OnCompleteFrame(std::move(test_frame)); // Verify that the per-packet information is passed to the renderer. EXPECT_THAT(fake_renderer_.WaitForFrame(kDefaultTimeOut), RenderedFrameWith(PacketInfos(ElementsAreArray(packet_infos)))); - int64_t timestamp_ms_max = clock_->TimeInMilliseconds(); + Timestamp timestamp_max = clock_->CurrentTime(); // Verify that the per-packet information also updates `GetSources()`. std::vector sources = video_receive_stream_->GetSources(); @@ -583,8 +583,8 @@ TEST_P(VideoReceiveStream2Test, RenderedFrameUpdatesGetSources) { EXPECT_EQ(it->source_id(), kSsrc); EXPECT_EQ(it->source_type(), RtpSourceType::SSRC); EXPECT_EQ(it->rtp_timestamp(), kRtpTimestamp); - EXPECT_GE(it->timestamp_ms(), timestamp_ms_min); - EXPECT_LE(it->timestamp_ms(), timestamp_ms_max); + EXPECT_GE(it->timestamp(), timestamp_min); + EXPECT_LE(it->timestamp(), timestamp_max); } { auto it = std::find_if(sources.begin(), sources.end(), @@ -596,8 +596,8 @@ TEST_P(VideoReceiveStream2Test, RenderedFrameUpdatesGetSources) { EXPECT_EQ(it->source_id(), kCsrc); EXPECT_EQ(it->source_type(), RtpSourceType::CSRC); EXPECT_EQ(it->rtp_timestamp(), kRtpTimestamp); - EXPECT_GE(it->timestamp_ms(), timestamp_ms_min); - EXPECT_LE(it->timestamp_ms(), timestamp_ms_max); + EXPECT_GE(it->timestamp(), timestamp_min); + EXPECT_LE(it->timestamp(), timestamp_max); } }