Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 2949553002: Wire up experiment for improved screenshare bwe.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 days, 6 hours ago by sprang_webrtc
Modified:
2 days, 6 hours ago
Reviewers:
philipel
CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhuangzesen_agora.io, zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Wire up experiment for improved screenshare bwe. Also adds some full stack test variants with the experiment enabled. BUG=webrtc:7694

Patch Set 1 #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+226 lines, -29 lines) Patch
M webrtc/modules/pacing/alr_detector.h View 3 chunks +22 lines, -1 line 0 comments Download
M webrtc/modules/pacing/alr_detector.cc View 3 chunks +52 lines, -12 lines 0 comments Download
M webrtc/modules/pacing/paced_sender.h View 2 chunks +6 lines, -0 lines 0 comments Download
M webrtc/modules/pacing/paced_sender.cc View 5 chunks +17 lines, -4 lines 0 comments Download
M webrtc/video/full_stack_tests.cc View 3 chunks +98 lines, -1 line 6 comments Download
M webrtc/video/screenshare_loopback.cc View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/video/video_loopback.cc View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/video/video_send_stream.cc View 9 chunks +29 lines, -9 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 5 (2 generated)
philipel
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.cc#oldcode313 webrtc/video/full_stack_tests.cc:313: test::ScopedFieldTrials field_trial("WebRTC-SimulcastScreenshare/Enabled/"); Do we just hijack some old experiment ...
2 days, 8 hours ago (2017-06-20 09:47:18 UTC) #3
sprang_webrtc
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.cc#oldcode313 webrtc/video/full_stack_tests.cc:313: test::ScopedFieldTrials field_trial("WebRTC-SimulcastScreenshare/Enabled/"); On 2017/06/20 09:47:18, philipel wrote: > Do ...
2 days, 7 hours ago (2017-06-20 10:48:59 UTC) #4
philipel
2 days, 6 hours ago (2017-06-20 12:14:31 UTC) #5
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318