Extend full stack tests with more stats

webrtc/video/full_stack.cc

Index: webrtc/video/full_stack.cc
diff --git a/webrtc/video/full_stack.cc b/webrtc/video/full_stack.cc
index ad1b5c6356557e3e4152b1dbd0f3c8c0f31ede2c..1f81276351745d39f2ec1e0fb4186c0b09bb0217 100644
--- a/webrtc/video/full_stack.cc
+++ b/webrtc/video/full_stack.cc
@@ -139,12 +139,14 @@ class VideoAnalyzer : public PacketReceiver,
     VideoFrame copy = video_frame;
     copy.set_timestamp(copy.ntp_time_ms() * 90);
 
+    VideoSendStream::Stats stats = send_stream_->GetStats();
     {
       rtc::CritScope lock(&crit_);
       if (first_send_frame_.IsZeroSize() && rtp_timestamp_delta_ == 0)
         first_send_frame_ = copy;
 
       frames_.push_back(copy);
+      send_stats_.push_back(stats);
     }
 
     input_->IncomingCapturedFrame(video_frame);
@@ -162,8 +164,11 @@ class VideoAnalyzer : public PacketReceiver,
             header.timestamp - first_send_frame_.timestamp();
         first_send_frame_.Reset();
       }
-      send_times_[header.timestamp - rtp_timestamp_delta_] =
+      uint32_t timestamp = header.timestamp - rtp_timestamp_delta_;
+      send_times_[timestamp] =
           Clock::GetRealTimeClock()->CurrentNtpInMilliseconds();
+      encoded_frame_sizes_[timestamp] +=

[pbos-webrtc, 2015/06/28 18:35:31]

Why do you need a += here if you remove the stats further down? This looks racy
if you want to catch all the combined simulcast layers.

[sprang_webrtc, 2015/07/06 11:19:00]

Is simulcast used in the full stack tests? Then how is send_times_ not racy?
The reason for the += is to capture the encoded size of a frame if it is
fragmented across several rtp packets. I'd rather have used
VideoSenderStream.post_encode_callback but EncodedFrameObserver takes an
EncodedFrame which doesn't give us anything to identify it by...
Any other ideas of how to solve this?
If it's even needed - I did this before adding the send stream stats, perhaps
the media send bitrate is enough..

[pbos-webrtc, 2015/07/06 11:36:00]

It won't be popped unless it's rendered, right? That requires all packets to be
delivered/given up. For one stream that's fine, we won't try to render it twice.
For simulcast we have 3 separate streams but one of them can be rendered before
the last one is even sent.

[sprang_webrtc, 2015/07/17 11:42:07]

True, what I'm saying is that send_times is racy as well then? Are we using
simulcast in these test?
Anyway, I removed this and instead have the pre_decode_callback directly add a
sample. This might not be entirely correct, but it'll have to be good enough.

+          length - (header.headerLength + header.paddingLength);
     }
 
     return transport_->SendRtp(packet, length);
@@ -183,17 +188,21 @@ class VideoAnalyzer : public PacketReceiver,
 
     while (frames_.front().timestamp() < send_timestamp) {
       AddFrameComparison(frames_.front(), last_rendered_frame_, true,
-                         render_time_ms);
+                         render_time_ms, send_stats_.front());
       frames_.pop_front();
+      send_stats_.pop_front();
     }
 
     VideoFrame reference_frame = frames_.front();
+    VideoSendStream::Stats send_stats = send_stats_.front();

[pbos-webrtc, 2015/06/28 18:35:32]

You can pop this one after AddFrameComparison so you don't need a temporary, up
to you though.

[sprang_webrtc, 2015/07/06 11:19:00]

Done.

     frames_.pop_front();
+    send_stats_.pop_front();
     assert(!reference_frame.IsZeroSize());
     EXPECT_EQ(reference_frame.timestamp(), send_timestamp);
     assert(reference_frame.timestamp() == send_timestamp);
 
-    AddFrameComparison(reference_frame, video_frame, false, render_time_ms);
+    AddFrameComparison(reference_frame, video_frame, false, render_time_ms,
+                       send_stats);
 
     last_rendered_frame_ = video_frame;
   }
@@ -227,24 +236,33 @@ class VideoAnalyzer : public PacketReceiver,
   VideoCaptureInput* input_;
   Transport* transport_;
   PacketReceiver* receiver_;
+  VideoSendStream* send_stream_;
 
  private:
   struct FrameComparison {
     FrameComparison()
-        : dropped(false), send_time_ms(0), recv_time_ms(0), render_time_ms(0) {}
+        : dropped(false),
+          send_time_ms(0),
+          recv_time_ms(0),
+          render_time_ms(0),
+          encoded_frame_size(0) {}
 
     FrameComparison(const VideoFrame& reference,
                     const VideoFrame& render,
                     bool dropped,
                     int64_t send_time_ms,
                     int64_t recv_time_ms,
-                    int64_t render_time_ms)
+                    int64_t render_time_ms,
+                    size_t encoded_frame_size,
+                    const VideoSendStream::Stats& send_stats)
         : reference(reference),
           render(render),
           dropped(dropped),
           send_time_ms(send_time_ms),
           recv_time_ms(recv_time_ms),
-          render_time_ms(render_time_ms) {}
+          render_time_ms(render_time_ms),
+          encoded_frame_size(encoded_frame_size),
+          send_stats(send_stats) {}
 
     VideoFrame reference;
     VideoFrame render;
@@ -252,25 +270,27 @@ class VideoAnalyzer : public PacketReceiver,
     int64_t send_time_ms;
     int64_t recv_time_ms;
     int64_t render_time_ms;
+    size_t encoded_frame_size;
+    VideoSendStream::Stats send_stats;
   };
 
   void AddFrameComparison(const VideoFrame& reference,
                           const VideoFrame& render,
                           bool dropped,
-                          int64_t render_time_ms)
+                          int64_t render_time_ms,
+                          const VideoSendStream::Stats& send_stats)
       EXCLUSIVE_LOCKS_REQUIRED(crit_) {
     int64_t send_time_ms = send_times_[reference.timestamp()];
     send_times_.erase(reference.timestamp());
     int64_t recv_time_ms = recv_times_[reference.timestamp()];
     recv_times_.erase(reference.timestamp());
+    size_t encoded_size = encoded_frame_sizes_[reference.timestamp()];
+    encoded_frame_sizes_.erase(reference.timestamp());
 
     rtc::CritScope crit(&comparison_lock_);
-    comparisons_.push_back(FrameComparison(reference,
-                                           render,
-                                           dropped,
-                                           send_time_ms,
-                                           recv_time_ms,
-                                           render_time_ms));
+    comparisons_.push_back(
+        FrameComparison(reference, render, dropped, send_time_ms, recv_time_ms,
+                        render_time_ms, encoded_size, send_stats));
     comparison_available_event_->Set();
   }
 
@@ -358,6 +378,12 @@ class VideoAnalyzer : public PacketReceiver,
     PrintResult("receiver_time", receiver_time_, " ms");
     PrintResult("total_delay_incl_network", end_to_end_, " ms");
     PrintResult("time_between_rendered_frames", rendered_delta_, " ms");
+    PrintResult("encoded_frame_size", encoded_frame_size_, " bytes");
+    PrintResult("encode_frame_rate_", encode_frame_rate_, " fps");
+    PrintResult("avg_encode_time", avg_encode_time_ms, " ms");
+    PrintResult("encode_usage_percent", encode_usage_percent, " percent");
+    PrintResult("media_bitrate", media_bitrate_bps, " bps");
+
     EXPECT_GT(psnr_.Mean(), avg_psnr_threshold_);
     EXPECT_GT(ssim_.Mean(), avg_ssim_threshold_);
   }
@@ -383,6 +409,11 @@ class VideoAnalyzer : public PacketReceiver,
     receiver_time_.AddSample(comparison.render_time_ms -
                              comparison.recv_time_ms);
     end_to_end_.AddSample(comparison.render_time_ms - input_time_ms);
+    encoded_frame_size_.AddSample(comparison.encoded_frame_size);
+    encode_frame_rate_.AddSample(comparison.send_stats.encode_frame_rate);

[pbos-webrtc, 2015/06/28 18:35:31]

Does this send_stats even necessarily correspond to the same frame? Otherwise I
see no reason to add it here, can't we just add samples from when we call
GetStats in the first place?

[sprang_webrtc, 2015/07/06 11:19:00]

The reason I did this was that I wanted samples from the same time frame as the
the other stats. If we do .AddSample when we capture a frame we might be adding
data points for a while after the we actually stop adding frame comparisons.

[pbos-webrtc, 2015/07/06 11:36:00]

Ok, so this essentially replaces a separate thread doing GetStats() every k ms,
and should not be corresponding to frame samples? I'm wondering whether we
should do that, WDYT?

[sprang_webrtc, 2015/07/17 11:42:07]

Sure. I added a check for frames_captured_ instead to limit scope.

+    avg_encode_time_ms.AddSample(comparison.send_stats.avg_encode_time_ms);
+    encode_usage_percent.AddSample(comparison.send_stats.encode_usage_percent);
+    media_bitrate_bps.AddSample(comparison.send_stats.media_bitrate_bps);
   }
 
   void PrintResult(const char* result_type,
@@ -403,6 +434,12 @@ class VideoAnalyzer : public PacketReceiver,
   test::Statistics ssim_;
   test::Statistics end_to_end_;
   test::Statistics rendered_delta_;
+  test::Statistics encoded_frame_size_;
+  test::Statistics encode_frame_rate_;
+  test::Statistics avg_encode_time_ms;
+  test::Statistics encode_usage_percent;
+  test::Statistics media_bitrate_bps;
+
   const int frames_to_process_;
   int frames_recorded_;
   int frames_processed_;
@@ -412,9 +449,11 @@ class VideoAnalyzer : public PacketReceiver,
 
   rtc::CriticalSection crit_;
   std::deque<VideoFrame> frames_ GUARDED_BY(crit_);
+  std::deque<VideoSendStream::Stats> send_stats_ GUARDED_BY(crit_);
   VideoFrame last_rendered_frame_ GUARDED_BY(crit_);
   std::map<uint32_t, int64_t> send_times_ GUARDED_BY(crit_);
   std::map<uint32_t, int64_t> recv_times_ GUARDED_BY(crit_);
+  std::map<uint32_t, size_t> encoded_frame_sizes_ GUARDED_BY(crit_);
   VideoFrame first_send_frame_ GUARDED_BY(crit_);
   const double avg_psnr_threshold_;
   const double avg_ssim_threshold_;
@@ -480,6 +519,7 @@ void FullStackTest::RunTest(const FullStackTestParams& params) {
 
   CreateStreams();
   analyzer.input_ = send_stream_->Input();
+  analyzer.send_stream_ = send_stream_;
 
   if (params.screenshare) {
     std::vector<std::string> slides;