Change reporting of timing frames conditions in GetStats on receive side
Instead of the longest frame since the last GetStats call, the longest
frame received during last 10 seconds should be returned in GetStats().
Previous way is not a good idea because there are maybe several
consumers of GetStats calls. If not all of them parse timing frame
reports, some of them may be lost.
Also, streamline reporting of TimingFrames via GetStats (remove separate
methods and use VideoReceiveStream::Stats struct instead).
BUG=webrtc:7594
Review-Url: https://codereview.webrtc.org/3008983002
Cr-Original-Commit-Position: refs/heads/master@{#19650}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: 75204c5ccdda561ccffbfa3a8fa94feac8e19e30
diff --git a/api/video/video_timing.cc b/api/video/video_timing.cc
index 0d22f90..2804c8b 100644
--- a/api/video/video_timing.cc
+++ b/api/video/video_timing.cc
@@ -39,6 +39,14 @@
return other_delay == -1 || EndToEndDelay() > other_delay;
}
+bool TimingFrameInfo::operator<(const TimingFrameInfo& other) const {
+ return other.IsLongerThan(*this);
+}
+
+bool TimingFrameInfo::operator<=(const TimingFrameInfo& other) const {
+ return !IsLongerThan(other);
+}
+
bool TimingFrameInfo::IsOutlier() const {
return !IsInvalid() && (flags & TimingFrameFlags::kTriggeredBySize);
}
@@ -54,15 +62,15 @@
std::string TimingFrameInfo::ToString() const {
std::stringstream out;
if (IsInvalid()) {
- out << "[Invalid]";
+ out << "";
} else {
out << rtp_timestamp << ',' << capture_time_ms << ',' << encode_start_ms
<< ',' << encode_finish_ms << ',' << packetization_finish_ms << ','
<< pacer_exit_ms << ',' << network_timestamp_ms << ','
<< network2_timestamp_ms << ',' << receive_start_ms << ','
<< receive_finish_ms << ',' << decode_start_ms << ','
- << decode_finish_ms << ',' << render_time_ms << ", outlier_triggered "
- << IsOutlier() << ", timer_triggered " << IsTimerTriggered();
+ << decode_finish_ms << ',' << render_time_ms << ','
+ << IsOutlier() << ',' << IsTimerTriggered();
}
return out.str();
}
diff --git a/api/video/video_timing.h b/api/video/video_timing.h
index f513463..e75c0c1 100644
--- a/api/video/video_timing.h
+++ b/api/video/video_timing.h
@@ -89,6 +89,10 @@
std::string ToString() const;
+ bool operator<(const TimingFrameInfo& other) const;
+
+ bool operator<=(const TimingFrameInfo& other) const;
+
uint32_t rtp_timestamp; // Identifier of a frame.
// All timestamps below are in local monotonous clock of a receiver.
// If sender clock is not yet estimated, sender timestamps
diff --git a/call/video_receive_stream.h b/call/video_receive_stream.h
index e78a369..542027e 100644
--- a/call/video_receive_stream.h
+++ b/call/video_receive_stream.h
@@ -95,6 +95,10 @@
StreamDataCounters rtp_stats;
RtcpPacketTypeCounter rtcp_packet_type_counts;
RtcpStatistics rtcp_stats;
+
+ // Timing frame info: all important timestamps for a full lifetime of a
+ // single 'timing frame'.
+ rtc::Optional<webrtc::TimingFrameInfo> timing_frame_info;
};
struct Config {
@@ -227,8 +231,6 @@
// TODO(pbos): Add info on currently-received codec to Stats.
virtual Stats GetStats() const = 0;
- virtual rtc::Optional<TimingFrameInfo> GetAndResetTimingFrameInfo() = 0;
-
// Takes ownership of the file, is responsible for closing it later.
// Calling this method will close and finalize any current log.
// Giving rtc::kInvalidPlatformFileValue disables logging.
diff --git a/media/engine/fakewebrtccall.cc b/media/engine/fakewebrtccall.cc
index af86fdd..8081819 100644
--- a/media/engine/fakewebrtccall.cc
+++ b/media/engine/fakewebrtccall.cc
@@ -309,11 +309,6 @@
return stats_;
}
-rtc::Optional<webrtc::TimingFrameInfo>
-FakeVideoReceiveStream::GetAndResetTimingFrameInfo() {
- return rtc::Optional<webrtc::TimingFrameInfo>();
-}
-
void FakeVideoReceiveStream::Start() {
receiving_ = true;
}
diff --git a/media/engine/fakewebrtccall.h b/media/engine/fakewebrtccall.h
index cd8d370..0b4496b 100644
--- a/media/engine/fakewebrtccall.h
+++ b/media/engine/fakewebrtccall.h
@@ -209,8 +209,6 @@
webrtc::VideoReceiveStream::Stats GetStats() const override;
- rtc::Optional<webrtc::TimingFrameInfo> GetAndResetTimingFrameInfo() override;
-
webrtc::VideoReceiveStream::Config config_;
bool receiving_;
webrtc::VideoReceiveStream::Stats stats_;
diff --git a/media/engine/webrtcvideoengine.cc b/media/engine/webrtcvideoengine.cc
index 4fabe52..cd34e6b 100644
--- a/media/engine/webrtcvideoengine.cc
+++ b/media/engine/webrtcvideoengine.cc
@@ -2434,7 +2434,7 @@
info.plis_sent = stats.rtcp_packet_type_counts.pli_packets;
info.nacks_sent = stats.rtcp_packet_type_counts.nack_packets;
- info.timing_frame_info = stream_->GetAndResetTimingFrameInfo();
+ info.timing_frame_info = stats.timing_frame_info;
if (log_stats)
LOG(LS_INFO) << stats.ToString(rtc::TimeMillis());
diff --git a/video/end_to_end_tests.cc b/video/end_to_end_tests.cc
index 8ace93e..107aed8 100644
--- a/video/end_to_end_tests.cc
+++ b/video/end_to_end_tests.cc
@@ -3631,7 +3631,7 @@
RunBaseTest(&test);
}
-TEST_F(EndToEndTest, GetTimingFrameInfoReportsTimingFrames) {
+TEST_F(EndToEndTest, TimingFramesAreReported) {
static const int kExtensionId = 5;
class StatsObserver : public test::EndToEndTest {
@@ -3639,12 +3639,6 @@
StatsObserver() : EndToEndTest(kLongTimeoutMs) {}
private:
- std::string CompoundKey(const char* name, uint32_t ssrc) {
- std::ostringstream oss;
- oss << name << "_" << ssrc;
- return oss.str();
- }
-
void ModifyVideoConfigs(
VideoSendStream::Config* send_config,
std::vector<VideoReceiveStream::Config>* receive_configs,
@@ -3668,13 +3662,13 @@
void PerformTest() override {
// No frames reported initially.
for (size_t i = 0; i < receive_streams_.size(); ++i) {
- EXPECT_FALSE(receive_streams_[i]->GetAndResetTimingFrameInfo());
+ EXPECT_FALSE(receive_streams_[i]->GetStats().timing_frame_info);
}
// Wait for at least one timing frame to be sent with 100ms grace period.
SleepMs(kDefaultTimingFramesDelayMs + 100);
// Check that timing frames are reported for each stream.
for (size_t i = 0; i < receive_streams_.size(); ++i) {
- EXPECT_TRUE(receive_streams_[i]->GetAndResetTimingFrameInfo());
+ EXPECT_TRUE(receive_streams_[i]->GetStats().timing_frame_info);
}
}
diff --git a/video/receive_statistics_proxy.cc b/video/receive_statistics_proxy.cc
index 6e58e1e..2448bad 100644
--- a/video/receive_statistics_proxy.cc
+++ b/video/receive_statistics_proxy.cc
@@ -104,7 +104,8 @@
freq_offset_counter_(clock, nullptr, kFreqOffsetProcessIntervalMs),
first_report_block_time_ms_(-1),
avg_rtt_ms_(0),
- last_content_type_(VideoContentType::UNSPECIFIED) {
+ last_content_type_(VideoContentType::UNSPECIFIED),
+ timing_frame_info_counter_(kMovingMaxWindowMs) {
stats_.ssrc = config_.rtp.remote_ssrc;
// TODO(brandtr): Replace |rtx_stats_| with a single instance of
// StreamDataCounters.
@@ -499,20 +500,10 @@
static_cast<int>(total_byte_tracker_.ComputeRate() * 8);
stats_.interframe_delay_max_ms =
interframe_delay_max_moving_.Max(now_ms).value_or(-1);
+ stats_.timing_frame_info = timing_frame_info_counter_.Max(now_ms);
return stats_;
}
-rtc::Optional<TimingFrameInfo>
-ReceiveStatisticsProxy::GetAndResetTimingFrameInfo() {
- rtc::CritScope lock(&crit_);
- rtc::Optional<TimingFrameInfo> info = timing_frame_info_;
- // Reset reported value to empty, so it will be always
- // overwritten in |OnTimingFrameInfoUpdated|, thus allowing to store new
- // value instead of reported one.
- timing_frame_info_.reset();
- return info;
-}
-
void ReceiveStatisticsProxy::OnIncomingPayloadType(int payload_type) {
rtc::CritScope lock(&crit_);
stats_.current_payload_type = payload_type;
@@ -557,13 +548,9 @@
void ReceiveStatisticsProxy::OnTimingFrameInfoUpdated(
const TimingFrameInfo& info) {
+ int64_t now_ms = clock_->TimeInMilliseconds();
rtc::CritScope lock(&crit_);
- // Only the frame which was processed the longest since the last
- // GetStats() call is reported. Therefore, only single 'longest' frame is
- // stored.
- if (!timing_frame_info_ || info.IsLongerThan(*timing_frame_info_)) {
- timing_frame_info_.emplace(info);
- }
+ timing_frame_info_counter_.Add(info, now_ms);
}
void ReceiveStatisticsProxy::RtcpPacketTypesCounterUpdated(
diff --git a/video/receive_statistics_proxy.h b/video/receive_statistics_proxy.h
index 9a6807c..140cdbd 100644
--- a/video/receive_statistics_proxy.h
+++ b/video/receive_statistics_proxy.h
@@ -48,8 +48,6 @@
VideoReceiveStream::Stats GetStats() const;
- rtc::Optional<TimingFrameInfo> GetAndResetTimingFrameInfo();
-
void OnDecodedFrame(rtc::Optional<uint8_t> qp, VideoContentType content_type);
void OnSyncOffsetUpdated(int64_t sync_offset_ms, double estimated_freq_khz);
void OnRenderedFrame(const VideoFrame& frame);
@@ -179,7 +177,10 @@
mutable std::map<int64_t, size_t> frame_window_ GUARDED_BY(&crit_);
VideoContentType last_content_type_ GUARDED_BY(&crit_);
rtc::Optional<int64_t> last_decoded_frame_time_ms_ GUARDED_BY(&crit_);
- rtc::Optional<TimingFrameInfo> timing_frame_info_ GUARDED_BY(&crit_);
+ // Mutable because calling Max() on MovingMaxCounter is not const. Yet it is
+ // called from const GetStats().
+ mutable rtc::MovingMaxCounter<TimingFrameInfo> timing_frame_info_counter_
+ GUARDED_BY(&crit_);
};
} // namespace webrtc
diff --git a/video/receive_statistics_proxy_unittest.cc b/video/receive_statistics_proxy_unittest.cc
index 528ef09..0b8b3a6 100644
--- a/video/receive_statistics_proxy_unittest.cc
+++ b/video/receive_statistics_proxy_unittest.cc
@@ -309,7 +309,7 @@
}
TEST_F(ReceiveStatisticsProxyTest,
- GetTimingFrameInfoReportsLongestTimingFrame) {
+ ReportsLongestTimingFrameInfo) {
const int64_t kShortEndToEndDelay = 10;
const int64_t kMedEndToEndDelay = 20;
const int64_t kLongEndToEndDelay = 100;
@@ -329,24 +329,28 @@
info.capture_time_ms = 0;
info.decode_finish_ms = kMedEndToEndDelay;
statistics_proxy_->OnTimingFrameInfoUpdated(info);
- result = statistics_proxy_->GetAndResetTimingFrameInfo();
+ result = statistics_proxy_->GetStats().timing_frame_info;
EXPECT_TRUE(result);
EXPECT_EQ(kExpectedRtpTimestamp, result->rtp_timestamp);
}
-TEST_F(ReceiveStatisticsProxyTest, GetTimingFrameInfoTimingFramesReportedOnce) {
+TEST_F(ReceiveStatisticsProxyTest, RespectsReportingIntervalForTimingFrames) {
+ TimingFrameInfo info;
const int64_t kShortEndToEndDelay = 10;
const uint32_t kExpectedRtpTimestamp = 2;
- TimingFrameInfo info;
+ const int64_t kShortDelayMs = 1000;
+ const int64_t kLongDelayMs = 10000;
rtc::Optional<TimingFrameInfo> result;
info.rtp_timestamp = kExpectedRtpTimestamp;
info.capture_time_ms = 0;
info.decode_finish_ms = kShortEndToEndDelay;
statistics_proxy_->OnTimingFrameInfoUpdated(info);
- result = statistics_proxy_->GetAndResetTimingFrameInfo();
+ fake_clock_.AdvanceTimeMilliseconds(kShortDelayMs);
+ result = statistics_proxy_->GetStats().timing_frame_info;
EXPECT_TRUE(result);
EXPECT_EQ(kExpectedRtpTimestamp, result->rtp_timestamp);
- result = statistics_proxy_->GetAndResetTimingFrameInfo();
+ fake_clock_.AdvanceTimeMilliseconds(kLongDelayMs);
+ result = statistics_proxy_->GetStats().timing_frame_info;
EXPECT_FALSE(result);
}
diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc
index 5af855b..9f5528b 100644
--- a/video/video_receive_stream.cc
+++ b/video/video_receive_stream.cc
@@ -251,11 +251,6 @@
return stats_proxy_.GetStats();
}
-rtc::Optional<TimingFrameInfo>
-VideoReceiveStream::GetAndResetTimingFrameInfo() {
- return stats_proxy_.GetAndResetTimingFrameInfo();
-}
-
void VideoReceiveStream::EnableEncodedFrameRecording(rtc::PlatformFile file,
size_t byte_limit) {
{
diff --git a/video/video_receive_stream.h b/video/video_receive_stream.h
index 967efcc..2f29c90 100644
--- a/video/video_receive_stream.h
+++ b/video/video_receive_stream.h
@@ -73,8 +73,6 @@
webrtc::VideoReceiveStream::Stats GetStats() const override;
- rtc::Optional<TimingFrameInfo> GetAndResetTimingFrameInfo() override;
-
// Takes ownership of the file, is responsible for closing it later.
// Calling this method will close and finalize any current log.
// Giving rtc::kInvalidPlatformFileValue disables logging.
