Espresso test for loopback video quality testing

Espresso test for loopback video quality testing

Update the AppRTCMobileTestStubbedVideoIO test to run on
phones without Internet connection. This is done by bringing up
a local instance of AppRTC on the Linux machine connected to
the Android device.

Running this test will need the webrtc.DEPS solution to be configured
for the checkout, since that will pull down the precompiled AppRTC
package that is needed.

Continued from http://crrev.com/2780493002#ps20001 (by kjellander@)
Continued from http://crrev.com/2741743002#ps180001 (by mandermo@)

BUG=webrtc:7185
Review-Url: https://codereview.webrtc.org/2825313002
Cr-Commit-Position: refs/heads/master@{#17838}
Committed: https://chromium.googlesource.com/external/webrtc/+/30cda5ef9823c0ba6783abf68b1a2c355e7382be

[oprypin_webrtc, 2017-04-19 14:54:25]

Description was changed from

==========
Espresso test for loopback video quality testing

Update the AppRTCMobileTestStubbedVideoIO test to run on
phones without Internet connection. This is done by bringing up
a local instance of AppRTC on the Linux machine connected to
the Android device.

Running this test will need the webrtc.DEPS solution to be configured
for the checkout, since that will pull down the precompiled AppRTC
package that is needed.

Continued from http://crrev.com/2780493002#ps20001

BUG=webrtc:7185
==========

to

==========
Espresso test for loopback video quality testing

Update the AppRTCMobileTestStubbedVideoIO test to run on
phones without Internet connection. This is done by bringing up
a local instance of AppRTC on the Linux machine connected to
the Android device.

Running this test will need the webrtc.DEPS solution to be configured
for the checkout, since that will pull down the precompiled AppRTC
package that is needed.

Continued from http://crrev.com/2780493002#ps20001
Continued from http://crrev.com/2741743002#ps180001

BUG=webrtc:7185
==========

[oprypin_webrtc, 2017-04-19 14:54:25]

oprypin@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[oprypin_webrtc, 2017-04-19 14:57:09]

This test does not properly run on swarming. We might just try and see what
happens on the perf bots. I have tried to eliminate everything that may go wrong
there, at least from the Python side of things.

Added kjellander@ as the only reviewer that was in the original CL, but we
probably need another reviewer for Java.

[oprypin_webrtc, 2017-04-19 14:57:55]

Description was changed from

==========
Espresso test for loopback video quality testing

Update the AppRTCMobileTestStubbedVideoIO test to run on
phones without Internet connection. This is done by bringing up
a local instance of AppRTC on the Linux machine connected to
the Android device.

Running this test will need the webrtc.DEPS solution to be configured
for the checkout, since that will pull down the precompiled AppRTC
package that is needed.

Continued from http://crrev.com/2780493002#ps20001
Continued from http://crrev.com/2741743002#ps180001

BUG=webrtc:7185
==========

to

==========
Espresso test for loopback video quality testing

Update the AppRTCMobileTestStubbedVideoIO test to run on
phones without Internet connection. This is done by bringing up
a local instance of AppRTC on the Linux machine connected to
the Android device.

Running this test will need the webrtc.DEPS solution to be configured
for the checkout, since that will pull down the precompiled AppRTC
package that is needed.

Continued from http://crrev.com/2780493002#ps20001 (by kjellander@)
Continued from http://crrev.com/2741743002#ps180001 (by mandermo@)

BUG=webrtc:7185
==========

[kjellander_webrtc, 2017-04-19 15:23:07]

kjellander@webrtc.org changed reviewers:
+ magjed@webrtc.org

[kjellander_webrtc, 2017-04-19 15:23:08]

Thanks for picking this up so we can finally get this going.

lgtm for *.py

I'm adding magjed@ for .java changes.

[magjed_webrtc, 2017-04-20 13:53:30]

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/AppRTCClient.java
(right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/AppRTCClient.java:39:
this(roomUrl, roomId, loopback, null);
nit: Add literal comment, i.e. null /* colliderUrl */

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java
(right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java:139:
builder.append("?debug=loopback");
Don't you need to use '&' instead of '?' for the following arguments?

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java:139:
builder.append("?debug=loopback");
This seems unrelated to colliderUrl. Maybe we should add a single String
parameter for all the url parameters?

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/sdk/android/api/or...
File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/sdk/android/api/or...
webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:144: try {
Why was this change necessary?

[oprypin_webrtc, 2017-04-20 15:46:15]

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/AppRTCClient.java
(right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/AppRTCClient.java:39:
this(roomUrl, roomId, loopback, null);
On 2017/04/20 13:53:30, magjed_webrtc wrote:
> nit: Add literal comment, i.e. null /* colliderUrl */

Will do.

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java
(right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java:139:
builder.append("?debug=loopback");
On 2017/04/20 13:53:30, magjed_webrtc wrote:
> Don't you need to use '&' instead of '?' for the following arguments?

Probably. I'll check if the test still works with &

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java:139:
builder.append("?debug=loopback");
On 2017/04/20 13:53:30, magjed_webrtc wrote:
> This seems unrelated to colliderUrl. Maybe we should add a single String
> parameter for all the url parameters?

To me this kinda makes sense - if collider URL is present, then enable some
debug flags, including passing collider URL.
I'm not sure what you mean about a single String parameter.

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/sdk/android/api/or...
File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/sdk/android/api/or...
webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:144: try {
On 2017/04/20 13:53:30, magjed_webrtc wrote:
> Why was this change necessary?

I really don't know. It comes from here:
https://codereview.chromium.org/2741743002/#ps1 - since it's the first patch
set, maybe the Description can tell you something about it.

[magjed_webrtc, 2017-04-21 08:23:09]

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java
(right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java:139:
builder.append("?debug=loopback");
On 2017/04/20 15:46:15, oprypin_webrtc wrote:
> On 2017/04/20 13:53:30, magjed_webrtc wrote:
> > This seems unrelated to colliderUrl. Maybe we should add a single String
> > parameter for all the url parameters?
> 
> To me this kinda makes sense - if collider URL is present, then enable some
> debug flags, including passing collider URL.
> I'm not sure what you mean about a single String parameter.

I was thinking we should have a String parameter 'urlParameters' instead of
'colliderUrl', and pass in all of "wstls=false&debug=loopback&ts=&wshpp=xxx".

Btw, why do we need 'debug=loopback'? We already have a boolean in
RoomConnectionParameters named 'loopback', why can't we use that?

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/sdk/android/api/or...
File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/sdk/android/api/or...
webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:144: try {
On 2017/04/20 15:46:15, oprypin_webrtc wrote:
> On 2017/04/20 13:53:30, magjed_webrtc wrote:
> > Why was this change necessary?
> 
> I really don't know. It comes from here:
> https://codereview.chromium.org/2741743002/#ps1 - since it's the first patch
> set, maybe the Description can tell you something about it.

I don't like this change, so if it works without, I want to revert it.

[oprypin_webrtc, 2017-04-24 10:08:06]

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java
(right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java:139:
builder.append("?debug=loopback");
On 2017/04/20 13:53:30, magjed_webrtc wrote:
> Don't you need to use '&' instead of '?' for the following arguments?

Turns out having all '?' works, and having '&' on following arguments does not.

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/sdk/android/api/or...
File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/sdk/android/api/or...
webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:144: try {
On 2017/04/21 08:23:09, magjed_webrtc wrote:
> On 2017/04/20 15:46:15, oprypin_webrtc wrote:
> > On 2017/04/20 13:53:30, magjed_webrtc wrote:
> > > Why was this change necessary?
> > 
> > I really don't know. It comes from here:
> > https://codereview.chromium.org/2741743002/#ps1 - since it's the first patch
> > set, maybe the Description can tell you something about it.
> 
> I don't like this change, so if it works without, I want to revert it.

Works without. Reverted.

[oprypin_webrtc, 2017-04-24 10:41:10]

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java
(right):

https://codereview.webrtc.org/2825313002/diff/60001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/WebSocketRTCClient.java:139:
builder.append("?debug=loopback");
On 2017/04/21 08:23:09, magjed_webrtc wrote:
> I was thinking we should have a String parameter 'urlParameters' instead of
> 'colliderUrl', and pass in all of "wstls=false&debug=loopback&ts=&wshpp=xxx".

Done.

> Btw, why do we need 'debug=loopback'? We already have a boolean in
> RoomConnectionParameters named 'loopback', why can't we use that?

RoomConnectionParameters loopback is indeed enabled, but this is a different
setting that is also needed -- this is actually being passed to the AppEngine
dev server, you can find out more about this complex interaction in the Python
script.

[magjed_webrtc, 2017-04-24 10:43:15]

lgtm

[oprypin_webrtc, 2017-04-24 10:58:07]

The CQ bit was checked by oprypin@webrtc.org

[oprypin_webrtc, 2017-04-24 10:58:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2825313002/#ps120001 (title:
"Rework colliderUrl as urlParameters")

[commit-bot: I haz the power, 2017-04-24 10:58:11]

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-04-24 11:15:14]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493031487637660,
"parent_rev": "828a6e1f018a6a170f0984051ea3603d9186be1a", "commit_rev":
"30cda5ef9823c0ba6783abf68b1a2c355e7382be"}

[commit-bot: I haz the power, 2017-04-24 11:15:17]

Description was changed from

==========
Espresso test for loopback video quality testing

Update the AppRTCMobileTestStubbedVideoIO test to run on
phones without Internet connection. This is done by bringing up
a local instance of AppRTC on the Linux machine connected to
the Android device.

Running this test will need the webrtc.DEPS solution to be configured
for the checkout, since that will pull down the precompiled AppRTC
package that is needed.

Continued from http://crrev.com/2780493002#ps20001 (by kjellander@)
Continued from http://crrev.com/2741743002#ps180001 (by mandermo@)

BUG=webrtc:7185
==========

to

==========
Espresso test for loopback video quality testing

Update the AppRTCMobileTestStubbedVideoIO test to run on
phones without Internet connection. This is done by bringing up
a local instance of AppRTC on the Linux machine connected to
the Android device.

Running this test will need the webrtc.DEPS solution to be configured
for the checkout, since that will pull down the precompiled AppRTC
package that is needed.

Continued from http://crrev.com/2780493002#ps20001 (by kjellander@)
Continued from http://crrev.com/2741743002#ps180001 (by mandermo@)

BUG=webrtc:7185

Review-Url: https://codereview.webrtc.org/2825313002
Cr-Commit-Position: refs/heads/master@{#17838}
Committed:
https://chromium.googlesource.com/external/webrtc/+/30cda5ef9823c0ba6783abf68...
==========

[commit-bot: I haz the power, 2017-04-24 11:15:18]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/30cda5ef9823c0ba6783abf68...