Adds a WebRTC browser_test with opus dtx enabled.

Adds a WebRTC browser_test with opus dtx enabled.

Additionally, starts tracking the 'packetsSentPerSecond' metric for WebRTC browser_tests.

BUG=631927
Committed: https://crrev.com/e9df939bf67e2dea8e3ca1d9a0857272324611b8
Cr-Commit-Position: refs/heads/master@{#408825}

[Ivo-OOO until feb 6, 2016-07-27 08:26:53]

Description was changed from

==========
Adds a WebRTC browser_test with opus dtx enabled.

Additionally, starts tracking the 'packetsSentPerSecond' metric for WebRTC
browser_tests.

BUG=631927
==========

to

==========
Adds a WebRTC browser_test with opus dtx enabled.

Additionally, starts tracking the 'packetsSentPerSecond' metric for WebRTC
browser_tests.

BUG=631927
==========

[Ivo-OOO until feb 6, 2016-07-27 08:26:53]

ivoc@chromium.org changed reviewers:
+ phoglund@chromium.org

[Ivo-OOO until feb 6, 2016-07-27 08:27:12]

ivoc@chromium.org changed reviewers:
+ minyue@chromium.org

[Ivo-OOO until feb 6, 2016-07-27 08:27:56]

Hi Patrik and Minyue,
I added a WebRTC browser_test to Chrome that uses Opus Dtx. Please have a look!

[phoglund_chromium, 2016-07-27 09:15:05]

https://codereview.chromium.org/2190533002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/2190533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest_base.cc:338: std::string
WebRtcTestBase::CreateLocalOffer(content::WebContents* from_tab,
I'm concerned this is going to go out of control if we keep adding more
parameters. Right now every configuration parameter goes through and clutters
createLocalOffer and other functions.

I think we have two options: either 

1) wrap codec and dtx in a config struct and pass as a js object, e.g.

struct WebRtcTestConfig {
  std::string default_video_codec;
  std::string opus_dtx;  // "true" or "false"

  std::string ToJavascript() {
    return base::StringPrintf("{ default_video_codec: %s, opus_dtx = %s }", 
                              default_video_codec, opus_dtx);
  }
};

Then the js code can just read config.opus_dtx.

or 2)

Make setters and global objects in js and remove variables from createoffer and
answer, e.g.

var gOpusDtx = false;

function forceOpusDtx() {
  gOpusDtx = true;
  returnToTest('ok-forced');
}

...

if (gOpusDtx) {
  munge sdp
}

and make a setter in webrtc_browsertest_base that calls

EXPECT_EQ("ok-forced", ExecuteJavascript("forceOpusDtx()"));

Then you can just call that setter from your test function after loading the
test page but before negotiating the call. In that case we should move
video_codec as well.

I think I prefer 2), what do you think?

https://codereview.chromium.org/2190533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest_base.cc:383: EXPECT_EQ("ok-",
response.substr(0, 3))
Just check ok-verified. In the above the response can be dynamic but you can
either get ok-verified or nothing right?

https://codereview.chromium.org/2190533002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_perf_browsertest.cc (right):

https://codereview.chromium.org/2190533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_perf_browsertest.cc:108: void
RunsAudioVideoCall60SecsAndLogsInternalMetrics(
You're not using opus_dtx for two-way calls, so remove the argument here. Just
do

NegotiateCall(left_tab, right_tab, video_codec, false /* opus_dtx */);

and remove the opus_dtx suffics from 145-148.

[Ivo-OOO until feb 6, 2016-07-27 14:12:02]

Patchset #2 (id:20001) has been deleted

[Ivo-OOO until feb 6, 2016-07-27 14:12:43]

Thanks for the comment, PTAL.

https://codereview.chromium.org/2190533002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/2190533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest_base.cc:338: std::string
WebRtcTestBase::CreateLocalOffer(content::WebContents* from_tab,
On 2016/07/27 09:15:05, phoglund_chrome wrote:
> I'm concerned this is going to go out of control if we keep adding more
> parameters. Right now every configuration parameter goes through and clutters
> createLocalOffer and other functions.
> 
> I think we have two options: either 
> 
> 1) wrap codec and dtx in a config struct and pass as a js object, e.g.
> 
> struct WebRtcTestConfig {
>   std::string default_video_codec;
>   std::string opus_dtx;  // "true" or "false"
> 
>   std::string ToJavascript() {
>     return base::StringPrintf("{ default_video_codec: %s, opus_dtx = %s }", 
>                               default_video_codec, opus_dtx);
>   }
> };
> 
> Then the js code can just read config.opus_dtx.
> 
> or 2)
> 
> Make setters and global objects in js and remove variables from createoffer
and
> answer, e.g.
> 
> var gOpusDtx = false;
> 
> function forceOpusDtx() {
>   gOpusDtx = true;
>   returnToTest('ok-forced');
> }
> 
> ...
> 
> if (gOpusDtx) {
>   munge sdp
> }
> 
> and make a setter in webrtc_browsertest_base that calls
> 
> EXPECT_EQ("ok-forced", ExecuteJavascript("forceOpusDtx()"));
> 
> Then you can just call that setter from your test function after loading the
> test page but before negotiating the call. In that case we should move
> video_codec as well.
> 
> I think I prefer 2), what do you think?

Good point, I implemented option 2.

https://codereview.chromium.org/2190533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest_base.cc:383: EXPECT_EQ("ok-",
response.substr(0, 3))
On 2016/07/27 09:15:05, phoglund_chrome wrote:
> Just check ok-verified. In the above the response can be dynamic but you can
> either get ok-verified or nothing right?

I moved this code to the javascript side, so this is no longer an issue.

https://codereview.chromium.org/2190533002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_perf_browsertest.cc (right):

https://codereview.chromium.org/2190533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_perf_browsertest.cc:108: void
RunsAudioVideoCall60SecsAndLogsInternalMetrics(
On 2016/07/27 09:15:05, phoglund_chrome wrote:
> You're not using opus_dtx for two-way calls, so remove the argument here. Just
> do
> 
> NegotiateCall(left_tab, right_tab, video_codec, false /* opus_dtx */);
> 
> and remove the opus_dtx suffics from 145-148.

Done.

[phoglund_chromium, 2016-07-28 06:39:02]

Very nice, just a couple small remaining comments.

https://codereview.chromium.org/2190533002/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc_browsertest_base.h (right):

https://codereview.chromium.org/2190533002/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest_base.h:170: // Add 'usedtx=1' to the
offer SDP.
Nit: blank line before #170

https://codereview.chromium.org/2190533002/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc_perf_browsertest.cc (right):

https://codereview.chromium.org/2190533002/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc_perf_browsertest.cc:200: *first_pc_dict, opus_dtx ?
"_sendonly_with_opus_dtx" : "_sendonly",
Maybe pull this out to a helper _MakePerfTestLabel("_sendonly", opus_dtx) and
use a proper if statement rather than ?.

https://codereview.chromium.org/2190533002/diff/40001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/peerconnection.js (right):

https://codereview.chromium.org/2190533002/diff/40001/chrome/test/data/webrtc...
chrome/test/data/webrtc/peerconnection.js:153:
verifyDefaultVideoCodec(answer.sdp, gDefaultVideoCodec);
I still want to keep the verification as a separate test call in order to keep
this function simple. What happened to dtx verification by the way?

[Ivo-OOO until feb 6, 2016-07-28 12:29:31]

https://codereview.chromium.org/2190533002/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc_browsertest_base.h (right):

https://codereview.chromium.org/2190533002/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest_base.h:170: // Add 'usedtx=1' to the
offer SDP.
On 2016/07/28 06:39:01, phoglund_chrome wrote:
> Nit: blank line before #170

Done.

https://codereview.chromium.org/2190533002/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc_perf_browsertest.cc (right):

https://codereview.chromium.org/2190533002/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc_perf_browsertest.cc:200: *first_pc_dict, opus_dtx ?
"_sendonly_with_opus_dtx" : "_sendonly",
On 2016/07/28 06:39:01, phoglund_chrome wrote:
> Maybe pull this out to a helper _MakePerfTestLabel("_sendonly", opus_dtx) and
> use a proper if statement rather than ?.

Done.

https://codereview.chromium.org/2190533002/diff/40001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/peerconnection.js (right):

https://codereview.chromium.org/2190533002/diff/40001/chrome/test/data/webrtc...
chrome/test/data/webrtc/peerconnection.js:153:
verifyDefaultVideoCodec(answer.sdp, gDefaultVideoCodec);
On 2016/07/28 06:39:01, phoglund_chrome wrote:
> I still want to keep the verification as a separate test call in order to keep
> this function simple. What happened to dtx verification by the way?

Ok, done. 
I realized that dtx verification is a bit useless, since we're actually setting
it manually in the setOpusDtxEnabled function below (which will generate a
failure if it runs into problems). So it was basically verifying the work done
by setOpusDtxEnabled, which doesn't seem terribly useful to me.

[Ivo-OOO until feb 6, 2016-07-28 12:31:27]

ivoc@chromium.org changed reviewers:
+ jochen@chromium.org

[Ivo-OOO until feb 6, 2016-07-28 12:33:35]

Adding jochen as reviewer for webrtc_event_log_apitest.cc, since grunell is OOO.

[phoglund_chromium, 2016-07-28 12:46:38]

lgtm % checkOpusDtxEnabled

https://codereview.chromium.org/2190533002/diff/60001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/munge_sdp.js (right):

https://codereview.chromium.org/2190533002/diff/60001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:56: function checkOpusDtxEnabled(sdp) {
Ok, then you don't need this anymore right?

[minyue, 2016-07-28 12:53:30]

https://codereview.chromium.org/2190533002/diff/60001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/munge_sdp.js (right):

https://codereview.chromium.org/2190533002/diff/60001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:41: // Find 'a=fmtp:111' line, where 111 is
the codecId
add findFmtpLine as a helper function

https://codereview.chromium.org/2190533002/diff/60001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:44: failure('setOpusDtxEnabled',
in this case, we may add "a=fmtp:" + |codecId| line

[Ivo-OOO until feb 6, 2016-07-28 14:00:07]

https://codereview.chromium.org/2190533002/diff/60001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/munge_sdp.js (right):

https://codereview.chromium.org/2190533002/diff/60001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:41: // Find 'a=fmtp:111' line, where 111 is
the codecId
On 2016/07/28 12:53:29, minyue wrote:
> add findFmtpLine as a helper function

Done.

https://codereview.chromium.org/2190533002/diff/60001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:44: failure('setOpusDtxEnabled',
On 2016/07/28 12:53:29, minyue wrote:
> in this case, we may add "a=fmtp:" + |codecId| line

Done.

https://codereview.chromium.org/2190533002/diff/60001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:56: function checkOpusDtxEnabled(sdp) {
On 2016/07/28 12:46:38, phoglund_chrome wrote:
> Ok, then you don't need this anymore right?

Good point, removed this.

[minyue, 2016-07-28 14:17:37]

lgtm

only nits

https://codereview.chromium.org/2190533002/diff/80001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/munge_sdp.js (right):

https://codereview.chromium.org/2190533002/diff/80001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:47: sdpLines.splice(rtpMapLine+1, 0,
newLine);
add spaces around +

https://codereview.chromium.org/2190533002/diff/80001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:49: else {
move up

https://codereview.chromium.org/2190533002/diff/80001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:190: */
Say return line number, and add @private

[Ivo-OOO until feb 6, 2016-07-28 14:42:38]

https://codereview.chromium.org/2190533002/diff/80001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/munge_sdp.js (right):

https://codereview.chromium.org/2190533002/diff/80001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:47: sdpLines.splice(rtpMapLine+1, 0,
newLine);
On 2016/07/28 14:17:37, minyue wrote:
> add spaces around +

Done.

https://codereview.chromium.org/2190533002/diff/80001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:49: else {
On 2016/07/28 14:17:37, minyue wrote:
> move up

Done.

https://codereview.chromium.org/2190533002/diff/80001/chrome/test/data/webrtc...
chrome/test/data/webrtc/munge_sdp.js:190: */
On 2016/07/28 14:17:37, minyue wrote:
> Say return line number, and add @private

Done.

[jochen (gone - plz use gerrit), 2016-07-29 09:41:05]

lgtm

[Ivo-OOO until feb 6, 2016-07-29 23:46:27]

The CQ bit was checked by ivoc@chromium.org

[Ivo-OOO until feb 6, 2016-07-29 23:46:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
phoglund@chromium.org, minyue@chromium.org
Link to the patchset: https://codereview.chromium.org/2190533002/#ps100001
(title: "Last comments by Minyue.")

[commit-bot: I haz the power, 2016-07-29 23:46:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-30 00:50:59]

Description was changed from

==========
Adds a WebRTC browser_test with opus dtx enabled.

Additionally, starts tracking the 'packetsSentPerSecond' metric for WebRTC
browser_tests.

BUG=631927
==========

to

==========
Adds a WebRTC browser_test with opus dtx enabled.

Additionally, starts tracking the 'packetsSentPerSecond' metric for WebRTC
browser_tests.

BUG=631927
==========

[commit-bot: I haz the power, 2016-07-30 00:51:00]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-07-30 00:54:14]

Description was changed from

==========
Adds a WebRTC browser_test with opus dtx enabled.

Additionally, starts tracking the 'packetsSentPerSecond' metric for WebRTC
browser_tests.

BUG=631927
==========

to

==========
Adds a WebRTC browser_test with opus dtx enabled.

Additionally, starts tracking the 'packetsSentPerSecond' metric for WebRTC
browser_tests.

BUG=631927

Committed: https://crrev.com/e9df939bf67e2dea8e3ca1d9a0857272324611b8
Cr-Commit-Position: refs/heads/master@{#408825}
==========

[commit-bot: I haz the power, 2016-07-30 00:54:16]

Patchset 5 (id:??) landed as
https://crrev.com/e9df939bf67e2dea8e3ca1d9a0857272324611b8
Cr-Commit-Position: refs/heads/master@{#408825}

[Guido Urdaneta, 2016-08-01 09:38:45]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/2195293002/ by guidou@chromium.org.

The reason for reverting is: Speculative revert due to this CL being suspect of
breaking webrtc bots.

See
https://build.chromium.org/p/chromium.webrtc/builders/Linux%20Tester/builds/1...

Sample error message:

[32431:32431:0801/012147:INFO:CONSOLE(13)] "Returning Test failed: Error:
getSdpDefaultCodec() failed: "'m=video' line missing from |sdp|."
    at failTest (http://127.0.0.1:47758/webrtc/test_functions.js:46:15)
    at failure (http://127.0.0.1:47758/webrtc/test_functions.js:56:9)
    at getSdpDefaultCodec (http://127.0.0.1:47758/webrtc/munge_sdp.js:108:5)
    at getSdpDefaultVideoCodec
(http://127.0.0.1:47758/webrtc/munge_sdp.js:93:10)
    at verifyDefaultVideoCodec
(http://127.0.0.1:47758/webrtc/peerconnection.js:174:27)
    at <anonymous>:1:51 to test.", source:
http://127.0.0.1:47758/webrtc/test_functions.js (13)
../../chrome/browser/media/webrtc_browsertest_base.cc:361: Failure.