Wire up experiment for improved screenshare bwe.

Wire up experiment for improved screenshare bwe.

Also adds some full stack test variants with the experiment enabled.

BUG=webrtc:7694
Review-Url: https://codereview.webrtc.org/2949553002
Cr-Commit-Position: refs/heads/master@{#18869}
Committed: https://chromium.googlesource.com/external/webrtc/+/89c4a7e57d524b13fbe0c823a83a4c10c2e63bd0

[sprang_webrtc, 2017-06-19 12:14:38]

Description was changed from

==========
Wire up experiment for improved screenshare bwe

BUG=webrtc:7694
==========

to

==========
Wire up experiment for improved screenshare bwe.

Also adds some full stack test variants with the experiment enabled.

BUG=webrtc:7694
==========

[sprang_webrtc, 2017-06-19 12:14:38]

sprang@webrtc.org changed reviewers:
+ philipel@webrtc.org

[philipel, 2017-06-20 09:47:18]

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests.cc
File webrtc/video/full_stack_tests.cc (left):

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests...
webrtc/video/full_stack_tests.cc:313: test::ScopedFieldTrials
field_trial("WebRTC-SimulcastScreenshare/Enabled/");
Do we just hijack some old experiment that we don't need to test anymore?

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests.cc
File webrtc/video/full_stack_tests.cc (right):

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests...
webrtc/video/full_stack_tests.cc:418: TEST_F(FullStackTest,
ScreenshareSlidesVP8_2TL_LossyNetRestrictedQueue_ALR) {
I'm not sure why we implement tests just for this experiment, why shouldn't it
stay after we are done with it?

Also, shouldn't we parameterize the FullStackTests (and EndToEndTests?).

[sprang_webrtc, 2017-06-20 10:48:59]

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests.cc
File webrtc/video/full_stack_tests.cc (left):

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests...
webrtc/video/full_stack_tests.cc:313: test::ScopedFieldTrials
field_trial("WebRTC-SimulcastScreenshare/Enabled/");
On 2017/06/20 09:47:18, philipel wrote:
> Do we just hijack some old experiment that we don't need to test anymore? 

This experiment is not old, it's currently being rolled out. I just made a
constant so we don't risk any copy/paste errors.

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests.cc
File webrtc/video/full_stack_tests.cc (right):

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests...
webrtc/video/full_stack_tests.cc:418: TEST_F(FullStackTest,
ScreenshareSlidesVP8_2TL_LossyNetRestrictedQueue_ALR) {
On 2017/06/20 09:47:18, philipel wrote:
> I'm not sure why we implement tests just for this experiment, why shouldn't it
> stay after we are done with it?

If the experiment turns out well, we should settle on which parameters to used
and make them constant, then remove the experiment wireup. So the "non
experiment" copies will take over.

> Also, shouldn't we parameterize the FullStackTests (and EndToEndTests?). 

Then we'll effectively double the number of longs test we're running, increasing
runtimes and adding even more load on the perf sheriffs that already have a
baszillion alerts to look at every week...

[philipel, 2017-06-20 12:14:31]

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests.cc
File webrtc/video/full_stack_tests.cc (left):

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests...
webrtc/video/full_stack_tests.cc:313: test::ScopedFieldTrials
field_trial("WebRTC-SimulcastScreenshare/Enabled/");
On 2017/06/20 10:48:58, sprang_webrtc wrote:
> On 2017/06/20 09:47:18, philipel wrote:
> > Do we just hijack some old experiment that we don't need to test anymore? 
> 
> This experiment is not old, it's currently being rolled out. I just made a
> constant so we don't risk any copy/paste errors.

ah...

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests.cc
File webrtc/video/full_stack_tests.cc (right):

https://codereview.webrtc.org/2949553002/diff/1/webrtc/video/full_stack_tests...
webrtc/video/full_stack_tests.cc:418: TEST_F(FullStackTest,
ScreenshareSlidesVP8_2TL_LossyNetRestrictedQueue_ALR) {
On 2017/06/20 10:48:58, sprang_webrtc wrote:
> On 2017/06/20 09:47:18, philipel wrote:
> > I'm not sure why we implement tests just for this experiment, why shouldn't
it
> > stay after we are done with it?
> 
> If the experiment turns out well, we should settle on which parameters to used
> and make them constant, then remove the experiment wireup. So the "non
> experiment" copies will take over.
> 

If these tests are copies of other tests then we should parameterize those tests
instead, is what I'm trying to say :)

> > Also, shouldn't we parameterize the FullStackTests (and EndToEndTests?). 
> 
> Then we'll effectively double the number of longs test we're running,
increasing
> runtimes and adding even more load on the perf sheriffs that already have a
> baszillion alerts to look at every week...

I think it might be interesting to run those tests before we remove this
experiment, but maybe we don't have to paramterize them yet.

[philipel, 2017-06-29 11:09:38]

after some offline discussion, lgtm

[philipel, 2017-06-29 11:10:12]

The CQ bit was checked by philipel@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 11:10:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-29 11:11:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 11:11:54]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/21561)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/21350)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26878)

[sprang_webrtc, 2017-06-29 14:57:56]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-06-29 14:57:57]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2949553002/#ps20001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-06-29 14:58:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-29 14:59:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 14:59:58]

Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/21574)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26891)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/25883)

[sprang_webrtc, 2017-06-29 15:05:57]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-06-29 15:05:57]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2949553002/#ps40001 (title:
"gn")

[commit-bot: I haz the power, 2017-06-29 15:06:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-29 15:12:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 15:12:19]

Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/22734)

[sprang_webrtc, 2017-06-29 15:22:30]

The CQ bit was checked by sprang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 15:22:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-29 15:53:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 15:53:13]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-06-29 17:43:46]

stefan@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2017-06-29 17:43:47]

https://codereview.webrtc.org/2949553002/diff/60001/webrtc/video/full_stack_t...
File webrtc/video/full_stack_tests.cc (right):

https://codereview.webrtc.org/2949553002/diff/60001/webrtc/video/full_stack_t...
webrtc/video/full_stack_tests.cc:34: "/1.1-2875-80-90/";
You have a double-/ here:

std::string(AlrDetector::kScreenshareProbingBweExperimentName) + 
"/1.1-2875-80-90/"
 = "WebRTC-SimulcastScreenshare/Enabled/" + "/1.1-2875-80-90/"

[sprang_webrtc, 2017-06-29 19:14:06]

https://codereview.webrtc.org/2949553002/diff/60001/webrtc/video/full_stack_t...
File webrtc/video/full_stack_tests.cc (right):

https://codereview.webrtc.org/2949553002/diff/60001/webrtc/video/full_stack_t...
webrtc/video/full_stack_tests.cc:34: "/1.1-2875-80-90/";
On 2017/06/29 17:43:47, stefan-webrtc wrote:
> You have a double-/ here:
> 
> std::string(AlrDetector::kScreenshareProbingBweExperimentName) + 
> "/1.1-2875-80-90/"
>  = "WebRTC-SimulcastScreenshare/Enabled/" + "/1.1-2875-80-90/"

I don't think so, it's not kScreenshareSimulcastExperiment that's concatenated
here - it's AlrDetector::kScreenshareProbingBweExperimentName:

const char* AlrDetector::kScreenshareProbingBweExperimentName =
"WebRTC-ProbingScreenshareBwe";

[stefan-webrtc, 2017-06-29 20:07:20]

lgtm

https://codereview.webrtc.org/2949553002/diff/60001/webrtc/video/full_stack_t...
File webrtc/video/full_stack_tests.cc (right):

https://codereview.webrtc.org/2949553002/diff/60001/webrtc/video/full_stack_t...
webrtc/video/full_stack_tests.cc:34: "/1.1-2875-80-90/";
On 2017/06/29 19:14:06, sprang_webrtc wrote:
> On 2017/06/29 17:43:47, stefan-webrtc wrote:
> > You have a double-/ here:
> > 
> > std::string(AlrDetector::kScreenshareProbingBweExperimentName) + 
> > "/1.1-2875-80-90/"
> >  = "WebRTC-SimulcastScreenshare/Enabled/" + "/1.1-2875-80-90/"
> 
> I don't think so, it's not kScreenshareSimulcastExperiment that's concatenated
> here - it's AlrDetector::kScreenshareProbingBweExperimentName:
> 
> const char* AlrDetector::kScreenshareProbingBweExperimentName =
> "WebRTC-ProbingScreenshareBwe";
> 

My bad :)

[sprang_webrtc, 2017-06-30 19:14:24]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-06-30 19:14:25]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2949553002/#ps60001 (title:
"sscanf format string, again")

[commit-bot: I haz the power, 2017-06-30 19:14:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-30 20:27:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498850064923130,
"parent_rev": "e96c45b662fc4b8cf7a4cac183461e522b91851e", "commit_rev":
"89c4a7e57d524b13fbe0c823a83a4c10c2e63bd0"}

[commit-bot: I haz the power, 2017-06-30 20:27:47]

Description was changed from

==========
Wire up experiment for improved screenshare bwe.

Also adds some full stack test variants with the experiment enabled.

BUG=webrtc:7694
==========

to

==========
Wire up experiment for improved screenshare bwe.

Also adds some full stack test variants with the experiment enabled.

BUG=webrtc:7694

Review-Url: https://codereview.webrtc.org/2949553002
Cr-Commit-Position: refs/heads/master@{#18869}
Committed:
https://chromium.googlesource.com/external/webrtc/+/89c4a7e57d524b13fbe0c823a...
==========

[commit-bot: I haz the power, 2017-06-30 20:27:49]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/89c4a7e57d524b13fbe0c823a...