Revert of Make the new jitter buffer the default jitter buffer. (patchset #2 id:290001 of https://codereview.chromium.org/2652043005/ )

Reason for revert:
Breaks downstream bots

Original issue's description:
> Reland of Make the new jitter buffer the default jitter buffer. (patchset #1 id:1 of https://codereview.webrtc.org/2638423003/ )
>
> Reason for revert:
> Bugfixes related to the new jitter buffer has landed.
>
> Original issue's description:
> > Revert of Make the new jitter buffer the default jitter buffer. (patchset #2 id:230001 of https://codereview.webrtc.org/2642753002/ )
> >
> > Reason for revert:
> > Breaks tests downstream.
> >
> > Original issue's description:
> > > Reland of Make the new jitter buffer the default jitter buffer. (patchset #1 id:1 of https://codereview.chromium.org/2632123005/ )
> > >
> > > Reason for revert:
> > > Fix in this CL: https://codereview.chromium.org/2640793003/
> > >
> > > Original issue's description:
> > > > Revert of Make the new jitter buffer the default jitter buffer. (patchset #7 id:120001 of https://codereview.chromium.org/2627463004/ )
> > > >
> > > > Reason for revert:
> > > > Breaks android bots.
> > > >
> > > > Original issue's description:
> > > > > Make the new jitter buffer the default jitter buffer.
> > > > >
> > > > > This CL contains only the changes necessary to make the switch to the new jitter
> > > > > buffer, clean up will be done in follow up CLs.
> > > > >
> > > > > In this CL:
> > > > >  - Removed the WebRTC-NewVideoJitterBuffer experiment and made the
> > > > >    new video jitter buffer the default one.
> > > > >  - Moved WebRTC.Video.KeyFramesReceivedInPermille and
> > > > >    WebRTC.Video.JitterBufferDelayInMs to the ReceiveStatisticsProxy.
> > > > >
> > > > > BUG=webrtc:5514
> > > > >
> > > > > Review-Url: https://codereview.webrtc.org/2627463004
> > > > > Cr-Commit-Position: refs/heads/master@{#16114}
> > > > > Committed: https://chromium.googlesource.com/external/webrtc/+/0f0763d86d5d4e7f27e8dece02560e39c6da97d6
> > > >
> > > > TBR=stefan@webrtc.org,terelius@webrtc.org
> > > > # Skipping CQ checks because original CL landed less than 1 days ago.
> > > > NOPRESUBMIT=true
> > > > NOTREECHECKS=true
> > > > NOTRY=true
> > > > BUG=webrtc:5514
> > > >
> > > > Review-Url: https://codereview.webrtc.org/2632123005
> > > > Cr-Commit-Position: refs/heads/master@{#16117}
> > > > Committed: https://chromium.googlesource.com/external/webrtc/+/c08c191f7d206dc0de945185370d18f29d556931
> > >
> > > TBR=stefan@webrtc.org,terelius@webrtc.org
> > > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > > BUG=webrtc:5514
> > >
> > > Review-Url: https://codereview.webrtc.org/2642753002
> > > Cr-Commit-Position: refs/heads/master@{#16149}
> > > Committed: https://chromium.googlesource.com/external/webrtc/+/f20dd0014d1cfc8a2e859a9e177e7fe2b21274ca
> >
> > TBR=stefan@webrtc.org,terelius@webrtc.org,philipel@webrtc.org
> > # Skipping CQ checks because original CL landed less than 1 days ago.
> > NOPRESUBMIT=true
> > NOTREECHECKS=true
> > NOTRY=true
> > BUG=webrtc:5514
> >
> > Review-Url: https://codereview.webrtc.org/2638423003
> > Cr-Commit-Position: refs/heads/master@{#16159}
> > Committed: https://chromium.googlesource.com/external/webrtc/+/04926b82641c426d764aa6e013e133db519129db
>
> TBR=stefan@webrtc.org,terelius@webrtc.org,kjellander@webrtc.org,kjellander@google.com
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=webrtc:5514
>
> Review-Url: https://codereview.webrtc.org/2652043005
> Cr-Commit-Position: refs/heads/master@{#16293}
> Committed: https://chromium.googlesource.com/external/webrtc/+/09d6ef00fc21b9f2c2c27e50e5e2952329ac4b4b

TBR=stefan@webrtc.org,terelius@webrtc.org,kjellander@webrtc.org,kjellander@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:5514

Review-Url: https://codereview.webrtc.org/2656983002
Cr-Commit-Position: refs/heads/master@{#16316}

Author: Philipel-WebRTC

webrtc/modules/video_coding/frame_buffer2.cc

@@ -16,7 +16,6 @@
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
-#include "webrtc/modules/video_coding/include/video_coding_defines.h"
 #include "webrtc/modules/video_coding/jitter_estimator.h"
 #include "webrtc/modules/video_coding/timing.h"
 #include "webrtc/system_wrappers/include/clock.h"
@@ -35,8 +34,7 @@ constexpr int kMaxFramesHistory = 50;
 
 FrameBuffer::FrameBuffer(Clock* clock,
                          VCMJitterEstimator* jitter_estimator,
-                         VCMTiming* timing,
-                         VCMReceiveStatisticsCallback* stats_callback)
+                         VCMTiming* timing)
     : clock_(clock),
       new_countinuous_frame_event_(false, false),
       jitter_estimator_(jitter_estimator),
@@ -47,10 +45,11 @@ FrameBuffer::FrameBuffer(Clock* clock,
       num_frames_history_(0),
       num_frames_buffered_(0),
       stopped_(false),
-      protection_mode_(kProtectionNack),
-      stats_callback_(stats_callback) {}
+      protection_mode_(kProtectionNack) {}
 
-FrameBuffer::~FrameBuffer() {}
+FrameBuffer::~FrameBuffer() {
+  UpdateHistograms();
+}
 
 FrameBuffer::ReturnReason FrameBuffer::NextFrame(
     int64_t max_wait_time_ms,
@@ -166,8 +165,9 @@ int FrameBuffer::InsertFrame(std::unique_ptr<FrameObject> frame) {
   rtc::CritScope lock(&crit_);
   RTC_DCHECK(frame);
 
-  if (stats_callback_)
-    stats_callback_->OnCompleteFrame(frame->num_references == 0, frame->size());
+  ++num_total_frames_;
+  if (frame->num_references == 0)
+    ++num_key_frames_;
 
   FrameKey key(frame->picture_id, frame->spatial_layer);
   int last_continuous_picture_id =
@@ -381,22 +381,28 @@ bool FrameBuffer::UpdateFrameInfoWithIncomingFrame(const FrameObject& frame,
 }
 
 void FrameBuffer::UpdateJitterDelay() {
-  if (!stats_callback_)
-    return;
-
-  int decode_ms;
-  int max_decode_ms;
-  int current_delay_ms;
-  int target_delay_ms;
-  int jitter_buffer_ms;
-  int min_playout_delay_ms;
-  int render_delay_ms;
-  if (timing_->GetTimings(&decode_ms, &max_decode_ms, &current_delay_ms,
-                          &target_delay_ms, &jitter_buffer_ms,
-                          &min_playout_delay_ms, &render_delay_ms)) {
-    stats_callback_->OnFrameBufferTimingsUpdated(
-        decode_ms, max_decode_ms, current_delay_ms, target_delay_ms,
-        jitter_buffer_ms, min_playout_delay_ms, render_delay_ms);
+  int unused;
+  int delay;
+  timing_->GetTimings(&unused, &unused, &unused, &unused, &delay, &unused,
+                      &unused);
+
+  accumulated_delay_ += delay;
+  ++accumulated_delay_samples_;
+}
+
+void FrameBuffer::UpdateHistograms() const {
+  rtc::CritScope lock(&crit_);
+  if (num_total_frames_ > 0) {
+    int key_frames_permille = (static_cast<float>(num_key_frames_) * 1000.0f /
+                                   static_cast<float>(num_total_frames_) +
+                               0.5f);
+    RTC_HISTOGRAM_COUNTS_1000("WebRTC.Video.KeyFramesReceivedInPermille",
+                              key_frames_permille);
+  }
+
+  if (accumulated_delay_samples_ > 0) {
+    RTC_HISTOGRAM_COUNTS_10000("WebRTC.Video.JitterBufferDelayInMs",
+                               accumulated_delay_ / accumulated_delay_samples_);
   }
 }
 

webrtc/modules/video_coding/frame_buffer2.h

@@ -28,7 +28,6 @@
 namespace webrtc {
 
 class Clock;
-class VCMReceiveStatisticsCallback;
 class VCMJitterEstimator;
 class VCMTiming;
 
@@ -40,8 +39,7 @@ class FrameBuffer {
 
   FrameBuffer(Clock* clock,
               VCMJitterEstimator* jitter_estimator,
-              VCMTiming* timing,
-              VCMReceiveStatisticsCallback* stats_proxy);
+              VCMTiming* timing);
 
   virtual ~FrameBuffer();
 
@@ -143,6 +141,8 @@ class FrameBuffer {
 
   void UpdateJitterDelay() EXCLUSIVE_LOCKS_REQUIRED(crit_);
 
+  void UpdateHistograms() const;
+
   void ClearFramesAndHistory() EXCLUSIVE_LOCKS_REQUIRED(crit_);
 
   FrameMap frames_ GUARDED_BY(crit_);
@@ -160,9 +160,16 @@ class FrameBuffer {
   int num_frames_buffered_ GUARDED_BY(crit_);
   bool stopped_ GUARDED_BY(crit_);
   VCMVideoProtection protection_mode_ GUARDED_BY(crit_);
-  VCMReceiveStatisticsCallback* const stats_callback_;
 
   RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(FrameBuffer);
+
+  // For WebRTC.Video.JitterBufferDelayInMs metric.
+  int64_t accumulated_delay_ = 0;
+  int64_t accumulated_delay_samples_ = 0;
+
+  // For WebRTC.Video.KeyFramesReceivedInPermille metric.
+  int64_t num_total_frames_ = 0;
+  int64_t num_key_frames_ = 0;
 };
 
 }  // namespace video_coding

webrtc/modules/video_coding/frame_buffer2_unittest.cc

@@ -25,9 +25,6 @@
 #include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
 
-using testing::_;
-using testing::Return;
-
 namespace webrtc {
 namespace video_coding {
 
@@ -57,16 +54,6 @@ class VCMTimingFake : public VCMTiming {
     return std::max<int>(0, render_time_ms - now_ms - kDecodeTime);
   }
 
-  bool GetTimings(int* decode_ms,
-                  int* max_decode_ms,
-                  int* current_delay_ms,
-                  int* target_delay_ms,
-                  int* jitter_buffer_ms,
-                  int* min_playout_delay_ms,
-                  int* render_delay_ms) const override {
-    return true;
-  }
-
  private:
   static constexpr int kDelayMs = 50;
   static constexpr int kDecodeTime = kDelayMs / 2;
@@ -95,27 +82,6 @@ class FrameObjectFake : public FrameObject {
   int64_t ReceivedTime() const override { return 0; }
 
   int64_t RenderTime() const override { return _renderTimeMs; }
-
-  // In EncodedImage |_length| is used to descibe its size and |_size| to
-  // describe its capacity.
-  void SetSize(int size) { _length = size; }
-};
-
-class VCMReceiveStatisticsCallbackMock : public VCMReceiveStatisticsCallback {
- public:
-  MOCK_METHOD2(OnReceiveRatesUpdated,
-               void(uint32_t bitRate, uint32_t frameRate));
-  MOCK_METHOD2(OnCompleteFrame, void(bool is_keyframe, size_t size_bytes));
-  MOCK_METHOD1(OnDiscardedPacketsUpdated, void(int discarded_packets));
-  MOCK_METHOD1(OnFrameCountsUpdated, void(const FrameCounts& frame_counts));
-  MOCK_METHOD7(OnFrameBufferTimingsUpdated,
-               void(int decode_ms,
-                    int max_decode_ms,
-                    int current_delay_ms,
-                    int target_delay_ms,
-                    int jitter_buffer_ms,
-                    int min_playout_delay_ms,
-                    int render_delay_ms));
 };
 
 class TestFrameBuffer2 : public ::testing::Test {
@@ -129,7 +95,7 @@ class TestFrameBuffer2 : public ::testing::Test {
       : clock_(0),
         timing_(&clock_),
         jitter_estimator_(&clock_),
-        buffer_(&clock_, &jitter_estimator_, &timing_, &stats_callback_),
+        buffer_(&clock_, &jitter_estimator_, &timing_),
         rand_(0x34678213),
         tear_down_(false),
         extract_thread_(&ExtractLoop, this, "Extract Thread"),
@@ -224,7 +190,6 @@ class TestFrameBuffer2 : public ::testing::Test {
   FrameBuffer buffer_;
   std::vector<std::unique_ptr<FrameObject>> frames_;
   Random rand_;
-  ::testing::NiceMock<VCMReceiveStatisticsCallbackMock> stats_callback_;
 
   int64_t max_wait_time_;
   bool tear_down_;
@@ -472,30 +437,5 @@ TEST_F(TestFrameBuffer2, PictureIdJumpBack) {
   CheckNoFrame(2);
 }
 
-TEST_F(TestFrameBuffer2, StatsCallback) {
-  uint16_t pid = Rand();
-  uint32_t ts = Rand();
-  const int kFrameSize = 5000;
-
-  EXPECT_CALL(stats_callback_, OnCompleteFrame(true, kFrameSize));
-  EXPECT_CALL(stats_callback_,
-              OnFrameBufferTimingsUpdated(_, _, _, _, _, _, _));
-
-  {
-    std::unique_ptr<FrameObjectFake> frame(new FrameObjectFake());
-    frame->SetSize(kFrameSize);
-    frame->picture_id = pid;
-    frame->spatial_layer = 0;
-    frame->timestamp = ts;
-    frame->num_references = 0;
-    frame->inter_layer_predicted = false;
-
-    EXPECT_EQ(buffer_.InsertFrame(std::move(frame)), pid);
-  }
-
-  ExtractFrame();
-  CheckFrame(0, pid, 0);
-}
-
 }  // namespace video_coding
 }  // namespace webrtc

webrtc/modules/video_coding/include/video_coding_defines.h

@@ -90,16 +90,8 @@ class VCMSendStatisticsCallback {
 class VCMReceiveStatisticsCallback {
  public:
   virtual void OnReceiveRatesUpdated(uint32_t bitRate, uint32_t frameRate) = 0;
-  virtual void OnCompleteFrame(bool is_keyframe, size_t size_bytes) = 0;
   virtual void OnDiscardedPacketsUpdated(int discarded_packets) = 0;
   virtual void OnFrameCountsUpdated(const FrameCounts& frame_counts) = 0;
-  virtual void OnFrameBufferTimingsUpdated(int decode_ms,
-                                           int max_decode_ms,
-                                           int current_delay_ms,
-                                           int target_delay_ms,
-                                           int jitter_buffer_ms,
-                                           int min_playout_delay_ms,
-                                           int render_delay_ms) = 0;
 
  protected:
   virtual ~VCMReceiveStatisticsCallback() {}

webrtc/modules/video_coding/timing.h

@@ -94,13 +94,13 @@ class VCMTiming {
 
   // Return current timing information. Returns true if the first frame has been
   // decoded, false otherwise.
-  virtual bool GetTimings(int* decode_ms,
-                          int* max_decode_ms,
-                          int* current_delay_ms,
-                          int* target_delay_ms,
-                          int* jitter_buffer_ms,
-                          int* min_playout_delay_ms,
-                          int* render_delay_ms) const;
+  bool GetTimings(int* decode_ms,
+                  int* max_decode_ms,
+                  int* current_delay_ms,
+                  int* target_delay_ms,
+                  int* jitter_buffer_ms,
+                  int* min_playout_delay_ms,
+                  int* render_delay_ms) const;
 
   enum { kDefaultRenderDelayMs = 10 };
   enum { kDelayMaxChangeMsPerS = 100 };

webrtc/modules/video_coding/video_receiver.cc

@@ -56,14 +56,31 @@ VideoReceiver::~VideoReceiver() {}
 
 void VideoReceiver::Process() {
   // Receive-side statistics
-
-  // TODO(philipel): Remove this if block when we know what to do with
-  //                 ReceiveStatisticsProxy::QualitySample.
   if (_receiveStatsTimer.TimeUntilProcess() == 0) {
     _receiveStatsTimer.Processed();
     rtc::CritScope cs(&process_crit_);
     if (_receiveStatsCallback != nullptr) {
-      _receiveStatsCallback->OnReceiveRatesUpdated(0, 0);
+      uint32_t bitRate;
+      uint32_t frameRate;
+      _receiver.ReceiveStatistics(&bitRate, &frameRate);
+      _receiveStatsCallback->OnReceiveRatesUpdated(bitRate, frameRate);
+    }
+
+    if (_decoderTimingCallback != nullptr) {
+      int decode_ms;
+      int max_decode_ms;
+      int current_delay_ms;
+      int target_delay_ms;
+      int jitter_buffer_ms;
+      int min_playout_delay_ms;
+      int render_delay_ms;
+      if (_timing->GetTimings(&decode_ms, &max_decode_ms, &current_delay_ms,
+                              &target_delay_ms, &jitter_buffer_ms,
+                              &min_playout_delay_ms, &render_delay_ms)) {
+        _decoderTimingCallback->OnDecoderTiming(
+            decode_ms, max_decode_ms, current_delay_ms, target_delay_ms,
+            jitter_buffer_ms, min_playout_delay_ms, render_delay_ms);
+      }
     }
   }
 
@@ -275,7 +292,7 @@ int32_t VideoReceiver::Decode(uint16_t maxWaitTimeMs) {
   return ret;
 }
 
-// Used for the new jitter buffer.
+// Used for the WebRTC-NewVideoJitterBuffer experiment.
 // TODO(philipel): Clean up among the Decode functions as we replace
 //                 VCMEncodedFrame with FrameObject.
 int32_t VideoReceiver::Decode(const webrtc::VCMEncodedFrame* frame) {

webrtc/video/end_to_end_tests.cc

@@ -1234,6 +1234,9 @@ TEST_P(EndToEndTest, ReceivesPliAndRecoversWithNack) {
 }
 
 TEST_P(EndToEndTest, ReceivesPliAndRecoversWithoutNack) {
+  // This test makes no sense for the new video jitter buffer.
+  if (GetParam() == new_jb_enabled)
+    return;
   ReceivesPliAndRecovers(0);
 }
 
@@ -3033,6 +3036,10 @@ TEST_P(EndToEndTest, GetStats) {
     ReceiveStreamRenderer receive_stream_renderer_;
   } test;
 
+  // TODO(philipel): Implement statistics for the new video jitter buffer.
+  if (GetParam() == new_jb_enabled)
+    return;
+
   RunBaseTest(&test);
 }