Make sure histograms in jitter buffer are only updated if running.
BUG=
R=pbos@webrtc.org, stefan@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/46089004
Cr-Commit-Position: refs/heads/master@{#9076}
diff --git a/webrtc/modules/video_coding/main/source/jitter_buffer.cc b/webrtc/modules/video_coding/main/source/jitter_buffer.cc
index 05b6c45..cd914be 100644
--- a/webrtc/modules/video_coding/main/source/jitter_buffer.cc
+++ b/webrtc/modules/video_coding/main/source/jitter_buffer.cc
@@ -171,7 +171,7 @@
}
void VCMJitterBuffer::UpdateHistograms() {
- if (num_packets_ <= 0) {
+ if (num_packets_ <= 0 || !running_) {
return;
}
int64_t elapsed_sec =
diff --git a/webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc b/webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc
index 42523e9..d70973e 100644
--- a/webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc
+++ b/webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc
@@ -20,6 +20,8 @@
#include "webrtc/modules/video_coding/main/source/test/stream_generator.h"
#include "webrtc/modules/video_coding/main/test/test_util.h"
#include "webrtc/system_wrappers/interface/clock.h"
+#include "webrtc/system_wrappers/interface/metrics.h"
+#include "webrtc/test/histogram.h"
namespace webrtc {
@@ -275,6 +277,48 @@
jitter_buffer_->ReleaseFrame(frame_out);
}
+TEST_F(TestBasicJitterBuffer, VerifyHistogramStats) {
+ test::ClearHistograms();
+ // Always start with a complete key frame when not allowing errors.
+ jitter_buffer_->SetDecodeErrorMode(kNoErrors);
+ packet_->frameType = kVideoFrameKey;
+ packet_->isFirstPacket = true;
+ packet_->markerBit = true;
+ packet_->timestamp += 123 * 90;
+
+ // Insert single packet frame to the jitter buffer and get a frame.
+ bool retransmitted = false;
+ EXPECT_EQ(kCompleteSession, jitter_buffer_->InsertPacket(*packet_,
+ &retransmitted));
+ VCMEncodedFrame* frame_out = DecodeCompleteFrame();
+ CheckOutFrame(frame_out, size_, false);
+ EXPECT_EQ(kVideoFrameKey, frame_out->FrameType());
+ jitter_buffer_->ReleaseFrame(frame_out);
+
+ // Verify that histograms are updated when the jitter buffer is stopped.
+ clock_->AdvanceTimeMilliseconds(metrics::kMinRunTimeInSeconds * 1000);
+ jitter_buffer_->Stop();
+ EXPECT_EQ(0, test::LastHistogramSample(
+ "WebRTC.Video.DiscardedPacketsInPercent"));
+ EXPECT_EQ(0, test::LastHistogramSample(
+ "WebRTC.Video.DuplicatedPacketsInPercent"));
+ EXPECT_NE(-1, test::LastHistogramSample(
+ "WebRTC.Video.CompleteFramesReceivedPerSecond"));
+ EXPECT_EQ(1000, test::LastHistogramSample(
+ "WebRTC.Video.KeyFramesReceivedInPermille"));
+
+ // Verify that histograms are not updated if stop is called again.
+ jitter_buffer_->Stop();
+ EXPECT_EQ(1, test::NumHistogramSamples(
+ "WebRTC.Video.DiscardedPacketsInPercent"));
+ EXPECT_EQ(1, test::NumHistogramSamples(
+ "WebRTC.Video.DuplicatedPacketsInPercent"));
+ EXPECT_EQ(1, test::NumHistogramSamples(
+ "WebRTC.Video.CompleteFramesReceivedPerSecond"));
+ EXPECT_EQ(1, test::NumHistogramSamples(
+ "WebRTC.Video.KeyFramesReceivedInPermille"));
+}
+
TEST_F(TestBasicJitterBuffer, DualPacketFrame) {
packet_->frameType = kVideoFrameKey;
packet_->isFirstPacket = true;
diff --git a/webrtc/test/histogram.cc b/webrtc/test/histogram.cc
index 4230715..0e6e9b2 100644
--- a/webrtc/test/histogram.cc
+++ b/webrtc/test/histogram.cc
@@ -12,6 +12,8 @@
#include <map>
+#include "webrtc/base/criticalsection.h"
+#include "webrtc/base/thread_annotations.h"
#include "webrtc/system_wrappers/interface/metrics.h"
// Test implementation of histogram methods in
@@ -19,8 +21,17 @@
namespace webrtc {
namespace {
-// Map holding the last added sample to a histogram (mapped by histogram name).
-std::map<std::string, int> histograms_;
+struct SampleInfo {
+ SampleInfo(int sample)
+ : last(sample), total(1) {}
+ int last; // Last added sample.
+ int total; // Total number of added samples.
+};
+
+rtc::CriticalSection histogram_crit_;
+// Map holding info about added samples to a histogram (mapped by the histogram
+// name).
+std::map<std::string, SampleInfo> histograms_ GUARDED_BY(histogram_crit_);
} // namespace
namespace metrics {
@@ -32,17 +43,39 @@
void HistogramAdd(
Histogram* histogram_pointer, const std::string& name, int sample) {
- histograms_[name] = sample;
+ rtc::CritScope cs(&histogram_crit_);
+ auto it = histograms_.find(name);
+ if (it == histograms_.end()) {
+ histograms_.insert(std::make_pair(name, SampleInfo(sample)));
+ return;
+ }
+ it->second.last = sample;
+ ++it->second.total;
}
} // namespace metrics
namespace test {
int LastHistogramSample(const std::string& name) {
- std::map<std::string, int>::const_iterator it = histograms_.find(name);
+ rtc::CritScope cs(&histogram_crit_);
+ const auto it = histograms_.find(name);
if (it == histograms_.end()) {
return -1;
}
- return it->second;
+ return it->second.last;
+}
+
+int NumHistogramSamples(const std::string& name) {
+ rtc::CritScope cs(&histogram_crit_);
+ const auto it = histograms_.find(name);
+ if (it == histograms_.end()) {
+ return 0;
+ }
+ return it->second.total;
+}
+
+void ClearHistograms() {
+ rtc::CritScope cs(&histogram_crit_);
+ histograms_.clear();
}
} // namespace test
} // namespace webrtc
diff --git a/webrtc/test/histogram.h b/webrtc/test/histogram.h
index 213fc8c..44ce32b 100644
--- a/webrtc/test/histogram.h
+++ b/webrtc/test/histogram.h
@@ -20,6 +20,12 @@
// found).
int LastHistogramSample(const std::string& name);
+// Returns the number of added samples to a histogram.
+int NumHistogramSamples(const std::string& name);
+
+// Removes all histograms.
+void ClearHistograms();
+
} // namespace test
} // namespace webrtc