Add experiment on weak ping delay during call set up time

Add experiment on weak ping delay during call set up time

BUG=
R=pthatcher@webrtc.org

Committed: https://crrev.com/3bf69b15f4c0c0ca4ab17c237084684a37bb8279
Cr-Commit-Position: refs/heads/master@{#10343}

[guoweis_webrtc, 2015-10-16 21:02:13]

guoweis@webrtc.org changed reviewers:
+ juberti@webrtc.org, pthatcher@webrtc.org

[guoweis_webrtc, 2015-10-16 21:02:14]

PTAL.

[pthatcher1, 2015-10-16 21:30:31]

https://codereview.webrtc.org/1411883002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1411883002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:226:
webrtc::field_trial::FindFullName("WebRTC-WeakPingDelay").c_str(),
While we call it "weak ping delay" internally, the name is a bit ugly.  I think
we may want a nicer one for the trial name.  How about
"WebRTC-StunInterPacketDelay"?

https://codereview.webrtc.org/1411883002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:228: weak_ping_delay_ = weak_ping_delay ?
weak_ping_delay : WEAK_PING_DELAY;
I think this would be more readable as:

if (weak_ping_delay) {
  weak_ping_delay_ = weak_ping_delay;
}

https://codereview.webrtc.org/1411883002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/1411883002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:254: int weak_ping_delay_ = 0;
I think it would be safer to make this = WEAK_PING_DELAY.

[guoweis_webrtc, 2015-10-16 21:49:06]

On 2015/10/16 21:30:31, pthatcher1 wrote:
>
https://codereview.webrtc.org/1411883002/diff/40001/webrtc/p2p/base/p2ptransp...
> File webrtc/p2p/base/p2ptransportchannel.cc (right):
> 
>
https://codereview.webrtc.org/1411883002/diff/40001/webrtc/p2p/base/p2ptransp...
> webrtc/p2p/base/p2ptransportchannel.cc:226:
> webrtc::field_trial::FindFullName("WebRTC-WeakPingDelay").c_str(),
> While we call it "weak ping delay" internally, the name is a bit ugly.  I
think
> we may want a nicer one for the trial name.  How about
> "WebRTC-StunInterPacketDelay"?
> 
>
https://codereview.webrtc.org/1411883002/diff/40001/webrtc/p2p/base/p2ptransp...
> webrtc/p2p/base/p2ptransportchannel.cc:228: weak_ping_delay_ = weak_ping_delay
?
> weak_ping_delay : WEAK_PING_DELAY;
> I think this would be more readable as:
> 
> if (weak_ping_delay) {
>   weak_ping_delay_ = weak_ping_delay;
> }
> 
>
https://codereview.webrtc.org/1411883002/diff/40001/webrtc/p2p/base/p2ptransp...
> File webrtc/p2p/base/p2ptransportchannel.h (right):
> 
>
https://codereview.webrtc.org/1411883002/diff/40001/webrtc/p2p/base/p2ptransp...
> webrtc/p2p/base/p2ptransportchannel.h:254: int weak_ping_delay_ = 0;
> I think it would be safer to make this = WEAK_PING_DELAY.

PTAL.

[pthatcher1, 2015-10-20 16:58:04]

lgtm

[guoweis_webrtc, 2015-10-20 17:05:13]

The CQ bit was checked by guoweis@webrtc.org

[commit-bot: I haz the power, 2015-10-20 17:05:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1411883002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1411883002/60001

[commit-bot: I haz the power, 2015-10-20 19:05:39]

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

[commit-bot: I haz the power, 2015-10-20 19:05:40]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[guoweis_webrtc, 2015-10-20 19:09:48]

Committed patchset #4 (id:60001) manually as
3bf69b15f4c0c0ca4ab17c237084684a37bb8279 (presubmit successful).

[commit-bot: I haz the power, 2015-10-20 19:09:53]

Patchset 4 (id:??) landed as
https://crrev.com/3bf69b15f4c0c0ca4ab17c237084684a37bb8279
Cr-Commit-Position: refs/heads/master@{#10343}

[kjellander_webrtc, 2015-10-21 06:54:05]

Description was changed from

==========
Add experiment on weak ping delay during call set up time

BUG=
R=pthatcher@webrtc.org

Committed: https://crrev.com/3bf69b15f4c0c0ca4ab17c237084684a37bb8279
Cr-Commit-Position: refs/heads/master@{#10343}
==========

to

==========
Add experiment on weak ping delay during call set up time

BUG=
R=pthatcher@webrtc.org

Committed: https://crrev.com/3bf69b15f4c0c0ca4ab17c237084684a37bb8279
Cr-Commit-Position: refs/heads/master@{#10343}
==========

[kjellander_webrtc, 2015-10-21 06:54:33]

On 2015/10/20 19:09:53, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/3bf69b15f4c0c0ca4ab17c237084684a37bb8279
> Cr-Commit-Position: refs/heads/master@{#10343}

On 2015/10/20 19:09:53, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/3bf69b15f4c0c0ca4ab17c237084684a37bb8279
> Cr-Commit-Position: refs/heads/master@{#10343}

I believe this broke the Chromium WebRTC FYI bots used for rolling:
[1168/1317] AR obj/chrome/libbrowser.a
FAILED: cd ../../remoting; python ../native_client/build/build_nexe.py --root ..
--product-dir ../out/Release/xyz --config-name Release -t
../native_client/toolchain/ --arch pnacl --build newlib_pexe --name
../out/Release/remoting_client_plugin_newlib.pexe --objdir
../out/Release/obj/remoting/remoting_client_plugin_nacl.gen/pnacl_newlib/remoting_client_plugin_nacl
"--include-dirs=../out/Release/gen/tc_pnacl_newlib/include ..
\"../out/Release/gen\" ../third_party/khronos ../gpu
../native_client_sdk/src/libraries
../native_client_sdk/src/libraries/nacl_io/include
../native_client_sdk/src/libraries/third_party/newlib-extras
../third_party/expat/files/lib ../third_party/icu/source/i18n
../third_party/icu/source/common ../third_party/libjingle/overrides
../third_party/libjingle/source ../third_party/webrtc_overrides ../third_party
../third_party/webrtc ../third_party/libyuv/include
../third_party/boringssl/src/include \"../out/Release/gen/protoc_out\"
../third_party/protobuf ../third_party/protobuf/src" "--compile_flags=-O2 -g
-Wall -fdiagnostics-show-option -Werror  -Wno-unused-function
-Wno-char-subscripts -Wno-c++11-extensions -Wno-unnamed-type-template-args
-Wno-extra-semi -Wno-unused-private-field -Wno-char-subscripts
-Wno-unused-function \"-std=gnu++11\" " --gomadir /b/build/goma
"--defines=\"__STDC_LIMIT_MACROS=1\" \"__STDC_FORMAT_MACROS=1\"
\"_GNU_SOURCE=1\" \"_DEFAULT_SOURCE=1\" \"_BSD_SOURCE=1\"
\"_POSIX_C_SOURCE=199506\" \"_XOPEN_SOURCE=600\"
\"DYNAMIC_ANNOTATIONS_ENABLED=1\" \"DYNAMIC_ANNOTATIONS_PREFIX=NACL_\"
\"NACL_BUILD_ARCH=x86\" V8_DEPRECATION_WARNINGS \"CLD_VERSION=2\"
\"_FILE_OFFSET_BITS=64\" CHROMIUM_BUILD \"CR_CLANG_REVISION=247874-1\"
UI_COMPOSITOR_IMAGE_TRANSPORT \"USE_AURA=1\" \"USE_ASH=1\" \"USE_PANGO=1\"
\"USE_CAIRO=1\" \"USE_DEFAULT_RENDER_THEME=1\" \"USE_LIBJPEG_TURBO=1\"
\"USE_X11=1\" \"USE_CLIPBOARD_AURAX11=1\" ENABLE_ONE_CLICK_SIGNIN
ENABLE_PRE_SYNC_BACKUP \"ENABLE_WEBRTC=1\" \"ENABLE_MEDIA_ROUTER=1\"
ENABLE_PEPPER_CDMS ENABLE_CONFIGURATION_POLICY ENABLE_NOTIFICATIONS
\"ENABLE_HIDPI=1\" \"ENABLE_TOPCHROME_MD=1\" USE_UDEV DONT_EMBED_BUILD_METADATA
\"DCHECK_ALWAYS_ON=1\" FIELDTRIAL_TESTING_ENABLED \"ENABLE_TASK_MANAGER=1\"
\"ENABLE_EXTENSIONS=1\" \"ENABLE_PDF=1\" \"ENABLE_PLUGINS=1\"
\"ENABLE_SESSION_SERVICE=1\" \"ENABLE_THEMES=1\" \"ENABLE_AUTOFILL_DIALOG=1\"
\"ENABLE_BACKGROUND=1\" \"ENABLE_GOOGLE_NOW=1\" \"ENABLE_PRINTING=1\"
\"ENABLE_BASIC_PRINTING=1\" \"ENABLE_PRINT_PREVIEW=1\" \"ENABLE_SPELLCHECK=1\"
\"ENABLE_CAPTIVE_PORTAL_DETECTION=1\" \"ENABLE_APP_LIST=1\"
\"ENABLE_SETTINGS_APP=1\" \"ENABLE_SUPERVISED_USERS=1\" \"ENABLE_MDNS=1\"
\"ENABLE_SERVICE_DISCOVERY=1\" V8_USE_EXTERNAL_STARTUP_DATA FULL_SAFE_BROWSING
SAFE_BROWSING_CSD SAFE_BROWSING_DB_LOCAL XML_STATIC \"U_USING_ICU_NAMESPACE=0\"
\"U_ENABLE_DYLOAD=0\" U_STATIC_IMPLEMENTATION EXPAT_RELATIVE_PATH
FEATURE_ENABLE_SSL GTEST_RELATIVE_PATH NO_MAIN_THREAD_WRAPPING NO_SOUND_SYSTEM
WEBRTC_POSIX SRTP_RELATIVE_PATH SSL_USE_OPENSSL USE_WEBRTC_DEV_BRANCH
GOOGLE_PROTOBUF_NO_RTTI GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER \"USE_LIBPCI=1\"
\"USE_OPENSSL=1\" \"USE_OPENSSL_CERTS=1\" __STDC_CONSTANT_MACROS
__STDC_FORMAT_MACROS" "--link_flags=-B../out/Release/gen/tc_pnacl_newlib/lib -O3
-lppapi_stub -lremoting_client_plugin_lib_nacl -lremoting_proto_nacl
-ljingle_glue_nacl -lnet_nacl -lcrypto_nacl -lbase_i18n_nacl -lbase_nacl
-lurl_nacl -lremoting_webrtc_nacl -lyuv_nacl -lvpx_nacl
-ljingle_p2p_constants_nacl -ljingle_nacl -lexpat_nacl -lmodp_b64_nacl
-lopus_nacl -lboringssl_nacl -licui18n_nacl -licuuc_nacl -licudata_nacl
-lprotobuf_lite_nacl -lppapi_cpp -lpthread -lnacl_io"
"--source-list=../out/gypfiles/remoting/pnacl_newlib.remoting_client_plugin_nacl.source_list.gypcmd"
../out/Release/gen/tc_pnacl_newlib/lib/libjingle_nacl.a: error: undefined
reference to 'webrtc::field_trial::FindFullName(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&)'

https://chromegw.corp.google.com/i/chromium.webrtc.fyi/builders/Linux/builds/...

You probably need to add a dependency on system_wrappers in the rtc_p2p target.

[tommi, 2015-10-21 08:07:10]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.webrtc.org/1423443002/ by tommi@webrtc.org.

The reason for reverting is: guoweis - Here's the target that's failing:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

This has unfortunately been causing problems repeatedly for us since
libjingle_nacl is maintained separately from libjingle (I don't know the
history).

The way this works for Chrome in general is that the FindFullName method is
implemented in init_webrtc.cc in the overrides folder in Chrome and that hooks
WebRTC up with Chrome's implementation.  I'm not sure if that's the right thing
to do for nacl, how webrtc is initialized there etc.  I'll ping the nacl team
for some help too offline and include you.  Reverting this change for now..

[hbos, 2015-10-21 08:36:13]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.webrtc.org/1416773003/ by hbos@webrtc.org.

The reason for reverting is: This CL breaks Chromium, undefined reference link
error to webrtc::field_trial::FindFullName. Adding the dependency
system_wrappers to the rtc_p2p target is not enough to fix this.

Looking at field_trial.h (in system_wrappers/interface/, not to be confused with
the one in test/) the documentation says "WebRTC clients MUST provide an
implementation of: ...FindFullName... Or link with a default one provided in:
...system_wrappers.gyp:field_trial_default).

So maybe just depend on field_trial_default? Not sure what should be done in
case there are implications to adding this dependency, I'm reverting this. Sorry
:).

[hbos, 2015-10-21 08:39:55]

On 2015/10/21 08:36:13, hbos wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.webrtc.org/1416773003/ by mailto:hbos@webrtc.org.
> 
> The reason for reverting is: This CL breaks Chromium, undefined reference link
> error to webrtc::field_trial::FindFullName. Adding the dependency
> system_wrappers to the rtc_p2p target is not enough to fix this.
> 
> Looking at field_trial.h (in system_wrappers/interface/, not to be confused
with
> the one in test/) the documentation says "WebRTC clients MUST provide an
> implementation of: ...FindFullName... Or link with a default one provided in:
> ...system_wrappers.gyp:field_trial_default).
> 
> So maybe just depend on field_trial_default? Not sure what should be done in
> case there are implications to adding this dependency, I'm reverting this.
Sorry
> :).

Oh you beat me to it tommi