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}

webrtc/modules/video_coding/main/source/jitter_buffer.cc

@@ -171,7 +171,7 @@ VCMJitterBuffer::~VCMJitterBuffer() {
 }
 
 void VCMJitterBuffer::UpdateHistograms() {
-  if (num_packets_ <= 0) {
+  if (num_packets_ <= 0 || !running_) {
     return;
   }
   int64_t elapsed_sec =

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 @@ TEST_F(TestBasicJitterBuffer, SinglePacketFrame) {
   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;

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).
+struct SampleInfo {
-std::map<std::string, int> histograms_;
+  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 @@ Histogram* HistogramFactoryGetEnumeration(const std::string& name,
 
 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

webrtc/test/histogram.h

@@ -20,6 +20,12 @@ namespace test {
 // 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