Performance measures of old and new RTCPeerConnection.getStats added.

chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc

Index: chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc
diff --git a/chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc b/chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc
index b4e42c2c9e389bffdc23f6c8c6053278cc81176b..b6a463a6d26b5f7a99f647a67edf4d10979f8b66 100644
--- a/chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc
+++ b/chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc
@@ -24,6 +24,11 @@ const char kMainWebrtcTestHtmlPage[] = "/webrtc/webrtc_jsep01_test.html";
 const char kInboundRtp[] = "inbound-rtp";
 const char kOutboundRtp[] = "outbound-rtp";
 
+enum class GetStatsVariety {

[hta - Chromium, 2017/01/23 14:53:52]

English nit: This should be "GetStatsVariation", not "Variety".

(I think the term for "variety" is a collective noun - meaning a set of things
that have differences. While "variation" is a single thing that is different
from something else.)

[hbos_chromium, 2017/01/24 12:08:36]

Done.

+  PROMISE_BASED,
+  CALLBACK_BASED
+};
+
 // Sums up "RTC[In/Out]boundRTPStreamStats.bytes_[received/sent]" values.
 double GetTotalRTPStreamBytes(
     TestStatsReportDictionary* report, const char* type,
@@ -84,36 +89,46 @@ class WebRtcStatsPerfBrowserTest : public WebRtcTestBase {
                                     "RTCPeerConnectionNewGetStats");
   }
 
-  void RunsAudioAndVideoCall(
-      const std::string& audio_codec, const std::string& video_codec) {
+  void StartCall(const std::string& audio_codec,
+                 const std::string& video_codec) {
     ASSERT_TRUE(test::HasReferenceFilesInCheckout());
     ASSERT_TRUE(embedded_test_server()->Start());
-    ASSERT_TRUE(audio_codec != kUseDefaultAudioCodec ||
-                video_codec != kUseDefaultVideoCodec);
 
     ASSERT_GE(TestTimeouts::action_max_timeout().InSeconds(), 100)
         << "This is a long-running test; you must specify "
            "--ui-test-action-max-timeout to have a value of at least 100000.";
 
-    content::WebContents* left_tab =
-        OpenTestPageAndGetUserMediaInNewTab(kMainWebrtcTestHtmlPage);
-    content::WebContents* right_tab =
-        OpenTestPageAndGetUserMediaInNewTab(kMainWebrtcTestHtmlPage);
-
-    SetupPeerconnectionWithLocalStream(left_tab);
-    SetupPeerconnectionWithLocalStream(right_tab);
-    SetDefaultAudioCodec(left_tab, audio_codec);
-    SetDefaultAudioCodec(right_tab, audio_codec);
-    SetDefaultVideoCodec(left_tab, video_codec);
-    SetDefaultVideoCodec(right_tab, video_codec);
-    NegotiateCall(left_tab, right_tab);
-    StartDetectingVideo(left_tab, "remote-view");
-    StartDetectingVideo(right_tab, "remote-view");
-    WaitForVideoToPlay(left_tab);
-    WaitForVideoToPlay(right_tab);
+    left_tab_ = OpenTestPageAndGetUserMediaInNewTab(kMainWebrtcTestHtmlPage);
+    right_tab_ = OpenTestPageAndGetUserMediaInNewTab(kMainWebrtcTestHtmlPage);
+
+    SetupPeerconnectionWithLocalStream(left_tab_);
+    SetupPeerconnectionWithLocalStream(right_tab_);
+    SetDefaultAudioCodec(left_tab_, audio_codec);
+    SetDefaultAudioCodec(right_tab_, audio_codec);
+    SetDefaultVideoCodec(left_tab_, video_codec);
+    SetDefaultVideoCodec(right_tab_, video_codec);
+    NegotiateCall(left_tab_, right_tab_);
+    StartDetectingVideo(left_tab_, "remote-view");
+    StartDetectingVideo(right_tab_, "remote-view");
+    WaitForVideoToPlay(left_tab_);
+    WaitForVideoToPlay(right_tab_);
+  }
+
+  void EndCall() {
+    if (left_tab_) {

[tommi (sloooow) - chröme, 2017/01/23 14:19:51]

nit: no need for {} in cases like this

[hbos_chromium, 2017/01/24 12:08:36]

Done.

+      HangUp(left_tab_);
+    }
+    if (right_tab_) {
+      HangUp(right_tab_);
+    }
+  }
+
+  void RunsAudioAndVideoCallCollectingMetrics(
+      const std::string& audio_codec, const std::string& video_codec) {
+    StartCall(audio_codec, video_codec);
 
     // Call for 60 seconds so that values may stabilize, bandwidth ramp up, etc.
-    test::SleepInJavascript(left_tab, 60000);
+    test::SleepInJavascript(left_tab_, 60000);
 
     // The ramp-up may vary greatly and impact the resulting total bytes, to get
     // reliable measurements we do two measurements, at 60 and 70 seconds and
@@ -124,7 +139,7 @@ class WebRtcStatsPerfBrowserTest : public WebRtcTestBase {
     double video_bytes_received_before = 0.0;
 
     scoped_refptr<TestStatsReportDictionary> report =
-        GetStatsReportDictionary(left_tab);
+        GetStatsReportDictionary(left_tab_);
     if (audio_codec != kUseDefaultAudioCodec) {
       audio_bytes_sent_before = GetAudioBytesSent(report.get());
       audio_bytes_received_before = GetAudioBytesReceived(report.get());
@@ -136,10 +151,10 @@ class WebRtcStatsPerfBrowserTest : public WebRtcTestBase {
     }
 
     double measure_duration_seconds = 10.0;
-    test::SleepInJavascript(left_tab, static_cast<int>(
+    test::SleepInJavascript(left_tab_, static_cast<int>(
         measure_duration_seconds * base::Time::kMillisecondsPerSecond));
 
-    report = GetStatsReportDictionary(left_tab);
+    report = GetStatsReportDictionary(left_tab_);
     if (audio_codec != kUseDefaultAudioCodec) {
       double audio_bytes_sent_after = GetAudioBytesSent(report.get());
       double audio_bytes_received_after = GetAudioBytesReceived(report.get());
@@ -179,62 +194,121 @@ class WebRtcStatsPerfBrowserTest : public WebRtcTestBase {
           "bytes/second", false);
     }
 
-    HangUp(left_tab);
-    HangUp(right_tab);
+    EndCall();
   }
+
+  void RunsAudioAndVideoCallMeasuringGetStatsPerformance(
+      GetStatsVariety variety) {
+    EXPECT_TRUE(base::TimeTicks::IsHighResolution());

[hta - Chromium, 2017/01/23 14:53:52]

From the peanut gallery: I see that you are using an "execute this Javascript"
procedure. Do you have a number for what the overhead of a do-nothing "execute
in Javascript" call is?

[hbos_chromium, 2017/01/24 12:08:36]

I moved the time measurements to the JS layer. This halved the performance
measurements, with 10+ ms overhead to "call javascript function from c++".

+
+    StartCall(kUseDefaultAudioCodec, kUseDefaultVideoCodec);
+
+    // Let the call run for 10 seconds to ensure stats not immediately available
+    // may be collected by our GetStats call.
+    test::SleepInJavascript(left_tab_, 10000);

[hta - Chromium, 2017/01/23 14:53:52]

Have you done some subjective testing to see if the test is actually affected by
this timer? 10 seconds sounds like a long time, and given the number of bots we
run, those seconds add up.

I'd suggest bisecting the space between 0 and 10 seconds in manual runs, find
the place where performance starts to change or become unstable, and pick a
number that's double that.
And then add a comment saying something like

const wait_length = 10000
// 10 seconds. We've seen performance vary if we only wait 5 seconds.

[hbos_chromium, 2017/01/24 12:08:36]

The idea was to get full coverage of stats, things like echo cancellation might
not have a value immediately, but this doesn't really matter. With manual
testing showing a high variation regardless of sleep time I removed the sleep
call.

+
+    base::TimeTicks ticks_before = base::TimeTicks::Now();
+    switch (variety) {
+      case GetStatsVariety::PROMISE_BASED:
+        GetStatsAndReturn(left_tab_);

[hta - Chromium, 2017/01/23 14:53:52]

Won't you get a semi-independent measure of the same thing if you also do
GetStatsAndReturn(right_tab_)?

[hbos_chromium, 2017/01/24 12:08:36]

Done, taking average of left and right tab's getStats.

+        break;
+      case GetStatsVariety::CALLBACK_BASED:
+        GetStatsCallbackAndReturn(left_tab_);
+        break;
+    }
+    base::TimeTicks ticks_after = base::TimeTicks::Now();
+    int64_t elapsed_us = (ticks_after - ticks_before).InMicroseconds();
+    perf_test::PrintResult(
+        "getStats",
+        (variety == GetStatsVariety::PROMISE_BASED) ? "_promise" : "_callback",
+        "invocation_time",
+        static_cast<double>(elapsed_us) /
+            base::Time::kMicrosecondsPerMillisecond,
+        "milliseconds",
+        false);
+
+    EndCall();
+  }
+
+ private:
+  content::WebContents* left_tab_ = nullptr;
+  content::WebContents* right_tab_ = nullptr;
 };
 
-IN_PROC_BROWSER_TEST_F(WebRtcStatsPerfBrowserTest,
-                       MANUAL_RunsAudioAndVideoCall_AudioCodec_opus) {
-  RunsAudioAndVideoCall("opus", kUseDefaultVideoCodec);
+IN_PROC_BROWSER_TEST_F(
+    WebRtcStatsPerfBrowserTest,
+    MANUAL_RunsAudioAndVideoCallCollectingMetrics_AudioCodec_opus) {
+  RunsAudioAndVideoCallCollectingMetrics("opus", kUseDefaultVideoCodec);
 }
 
-IN_PROC_BROWSER_TEST_F(WebRtcStatsPerfBrowserTest,
-                       MANUAL_RunsAudioAndVideoCall_AudioCodec_ISAC) {
-  RunsAudioAndVideoCall("ISAC", kUseDefaultVideoCodec);
+IN_PROC_BROWSER_TEST_F(
+    WebRtcStatsPerfBrowserTest,
+    MANUAL_RunsAudioAndVideoCallCollectingMetrics_AudioCodec_ISAC) {
+  RunsAudioAndVideoCallCollectingMetrics("ISAC", kUseDefaultVideoCodec);
 }
 
-IN_PROC_BROWSER_TEST_F(WebRtcStatsPerfBrowserTest,
-                       MANUAL_RunsAudioAndVideoCall_AudioCodec_G722) {
-  RunsAudioAndVideoCall("G722", kUseDefaultVideoCodec);
+IN_PROC_BROWSER_TEST_F(
+    WebRtcStatsPerfBrowserTest,
+    MANUAL_RunsAudioAndVideoCallCollectingMetrics_AudioCodec_G722) {
+  RunsAudioAndVideoCallCollectingMetrics("G722", kUseDefaultVideoCodec);
 }
 
-IN_PROC_BROWSER_TEST_F(WebRtcStatsPerfBrowserTest,
-                       MANUAL_RunsAudioAndVideoCall_AudioCodec_PCMU) {
-  RunsAudioAndVideoCall("PCMU", kUseDefaultVideoCodec);
+IN_PROC_BROWSER_TEST_F(
+    WebRtcStatsPerfBrowserTest,
+    MANUAL_RunsAudioAndVideoCallCollectingMetrics_AudioCodec_PCMU) {
+  RunsAudioAndVideoCallCollectingMetrics("PCMU", kUseDefaultVideoCodec);
 }
 
-IN_PROC_BROWSER_TEST_F(WebRtcStatsPerfBrowserTest,
-                       MANUAL_RunsAudioAndVideoCall_AudioCodec_PCMA) {
-  RunsAudioAndVideoCall("PCMA", kUseDefaultVideoCodec);
+IN_PROC_BROWSER_TEST_F(
+    WebRtcStatsPerfBrowserTest,
+    MANUAL_RunsAudioAndVideoCallCollectingMetrics_AudioCodec_PCMA) {
+  RunsAudioAndVideoCallCollectingMetrics("PCMA", kUseDefaultVideoCodec);
 }
 
-IN_PROC_BROWSER_TEST_F(WebRtcStatsPerfBrowserTest,
-                       MANUAL_RunsAudioAndVideoCall_VideoCodec_VP8) {
-  RunsAudioAndVideoCall(kUseDefaultAudioCodec, "VP8");
+IN_PROC_BROWSER_TEST_F(
+    WebRtcStatsPerfBrowserTest,
+    MANUAL_RunsAudioAndVideoCallCollectingMetrics_VideoCodec_VP8) {
+  RunsAudioAndVideoCallCollectingMetrics(kUseDefaultAudioCodec, "VP8");
 }
 
-IN_PROC_BROWSER_TEST_F(WebRtcStatsPerfBrowserTest,
-                       MANUAL_RunsAudioAndVideoCall_VideoCodec_VP9) {
-  RunsAudioAndVideoCall(kUseDefaultAudioCodec, "VP9");
+IN_PROC_BROWSER_TEST_F(
+    WebRtcStatsPerfBrowserTest,
+    MANUAL_RunsAudioAndVideoCallCollectingMetrics_VideoCodec_VP9) {
+  RunsAudioAndVideoCallCollectingMetrics(kUseDefaultAudioCodec, "VP9");
 }
 
 #if BUILDFLAG(RTC_USE_H264)
 
-IN_PROC_BROWSER_TEST_F(WebRtcStatsPerfBrowserTest,
-                       MANUAL_RunsAudioAndVideoCall_VideoCodec_H264) {
+IN_PROC_BROWSER_TEST_F(
+    WebRtcStatsPerfBrowserTest,
+    MANUAL_RunsAudioAndVideoCallCollectingMetrics_VideoCodec_H264) {
   // Only run test if run-time feature corresponding to |rtc_use_h264| is on.
   if (!base::FeatureList::IsEnabled(content::kWebRtcH264WithOpenH264FFmpeg)) {
     LOG(WARNING) << "Run-time feature WebRTC-H264WithOpenH264FFmpeg disabled. "
         "Skipping WebRtcPerfBrowserTest."
-        "MANUAL_RunsAudioAndVideoCall_VideoCodec_H264 (test \"OK\")";
+        "MANUAL_RunsAudioAndVideoCallCollectingMetrics_VideoCodec_H264 (test "
+        "\"OK\")";
     return;
   }
-  RunsAudioAndVideoCall(kUseDefaultAudioCodec, "H264");
+  RunsAudioAndVideoCallCollectingMetrics(kUseDefaultAudioCodec, "H264");
 }
 
 #endif  // BUILDFLAG(RTC_USE_H264)
 
+IN_PROC_BROWSER_TEST_F(
+    WebRtcStatsPerfBrowserTest,
+    MANUAL_RunsAudioAndVideoCallMeasuringGetStatsPerformance_Promise) {
+  RunsAudioAndVideoCallMeasuringGetStatsPerformance(
+      GetStatsVariety::PROMISE_BASED);
+}
+
+IN_PROC_BROWSER_TEST_F(
+    WebRtcStatsPerfBrowserTest,
+    MANUAL_RunsAudioAndVideoCallMeasuringGetStatsPerformance_Callback) {
+  RunsAudioAndVideoCallMeasuringGetStatsPerformance(
+      GetStatsVariety::CALLBACK_BASED);
+}
+
 }  // namespace
 
 }  // namespace content