Replace voe_conference_test.

- Removes voe_conference_test.
- Adds a new AudioStatsTest, with better coverage of the same features, based on call_test.
- Adds an AudioEndToEndTest utility, which AudioStatsTest and LowBandwidthAudioTest uses.

BUG=webrtc:4690
R=kwiberg@webrtc.org

Review-Url: https://codereview.webrtc.org/3008273002 .
Cr-Commit-Position: refs/heads/master@{#19833}
Committed: https://webrtc.googlesource.com/src/+/73276ad7ed1e70ab764cd02d7189ed5839fadc20

[the sun, 2017-09-08 13:51:47]

Description was changed from

==========
rebase

BUG=
==========

to

==========
- Removes voe_conference_test.
- Adds a new AudioStatsTest, with better coverage of the same features, based on
call_test.
- Adds an AudioEndToEndTest utility, which AudioStatsTest and
LowBandwidthAudioTest uses.

BUG=webrtc:4690
==========

[the sun, 2017-09-12 12:23:28]

solenberg@webrtc.org changed reviewers:
+ ossu@webrtc.org

[the sun, 2017-09-12 12:49:26]

solenberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org
- ossu@webrtc.org

[kwiberg-webrtc, 2017-09-13 14:20:26]

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/BUILD.gn
File webrtc/audio/BUILD.gn (right):

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/BUILD.gn#ne...
webrtc/audio/BUILD.gn:114: "test/audio_end_to_end_test.h",
It looks like you're adding the same files to more than one build target. I
don't think you're supposed to do that.

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
File webrtc/audio/test/audio_stats_test.cc (right):

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
webrtc/audio/test/audio_stats_test.cc:21: int error = reference / 10 + 1;
const?

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
webrtc/audio/test/audio_stats_test.cc:30: const int64_t kRttMs = 100;
static constexpr? Or maybe just constexpr, if you don't want to deal with
defining them again outside the class...

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
webrtc/audio/test/audio_stats_test.cc:52: EXPECT_EQ("OPUS",
send_stats.codec_name);
Should this comparison be case-insensitive? SDP codec names are case-insensitive
IIRC. Also, unlike all the other codecs, Opus's canonical name is lower-case...

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
webrtc/audio/test/audio_stats_test.cc:105:
static_cast<int32_t>(recv_stats.ext_seqnum));
static_cast is dangerous. Can you convert both of them to a larger type instead,
using e.g. int64_t{x}? Otherwise, I suggest checked_cast, or SafeEq.

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:156: task_queue_.SendTask([this, test]() {
The lambda is capturing test by value, i.e. making a copy. Is that what you
want?

[the sun, 2017-09-13 14:53:33]

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/BUILD.gn
File webrtc/audio/BUILD.gn (right):

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/BUILD.gn#ne...
webrtc/audio/BUILD.gn:114: "test/audio_end_to_end_test.h",
On 2017/09/13 14:20:26, kwiberg-webrtc wrote:
> It looks like you're adding the same files to more than one build target. I
> don't think you're supposed to do that.

Thanks!

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
File webrtc/audio/test/audio_stats_test.cc (right):

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
webrtc/audio/test/audio_stats_test.cc:21: int error = reference / 10 + 1;
On 2017/09/13 14:20:26, kwiberg-webrtc wrote:
> const?

Done.

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
webrtc/audio/test/audio_stats_test.cc:30: const int64_t kRttMs = 100;
On 2017/09/13 14:20:26, kwiberg-webrtc wrote:
> static constexpr? Or maybe just constexpr, if you don't want to deal with
> defining them again outside the class...

constexpr on its own yields compiler errors:

../../webrtc/audio/test/audio_stats_test.cc:27:3: error: non-static data member
cannot be constexpr; did you intend to make it static?
  constexpr int kTestDurationMs = 8000;
  ^
  static

and static constexpr yields a link errors:

Undefined symbols for architecture x86_64:
  "webrtc::test::(anonymous namespace)::NoLossTest::kBytesSent", referenced
from:
      webrtc::test::(anonymous namespace)::NoLossTest::OnStreamsStopped() in
audio_stats_test.o

I guess I'd have to redeclare the static constexpr outside the class
declaration, but then simply using 'const' seemed less wordy...

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
webrtc/audio/test/audio_stats_test.cc:52: EXPECT_EQ("OPUS",
send_stats.codec_name);
On 2017/09/13 14:20:26, kwiberg-webrtc wrote:
> Should this comparison be case-insensitive? SDP codec names are
case-insensitive
> IIRC. Also, unlike all the other codecs, Opus's canonical name is
lower-case...

Changing it to lower case, here and in call_test.cc

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
webrtc/audio/test/audio_stats_test.cc:105:
static_cast<int32_t>(recv_stats.ext_seqnum));
On 2017/09/13 14:20:26, kwiberg-webrtc wrote:
> static_cast is dangerous. Can you convert both of them to a larger type
instead,
> using e.g. int64_t{x}? Otherwise, I suggest checked_cast, or SafeEq.

Done.

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:156: task_queue_.SendTask([this, test]() {
On 2017/09/13 14:20:26, kwiberg-webrtc wrote:
> The lambda is capturing test by value, i.e. making a copy. Is that what you
> want?

test is a pointer and CallTest owns the TQ, so this task should execute before
the TEST_F defined test case is destroyed.

[kwiberg-webrtc, 2017-09-14 01:26:41]

lgtm

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
File webrtc/audio/test/audio_stats_test.cc (right):

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/audio/test/audio_...
webrtc/audio/test/audio_stats_test.cc:30: const int64_t kRttMs = 100;
On 2017/09/13 14:53:32, the sun wrote:
> On 2017/09/13 14:20:26, kwiberg-webrtc wrote:
> > static constexpr? Or maybe just constexpr, if you don't want to deal with
> > defining them again outside the class...
> 
> constexpr on its own yields compiler errors:
> 
> ../../webrtc/audio/test/audio_stats_test.cc:27:3: error: non-static data
member
> cannot be constexpr; did you intend to make it static?
>   constexpr int kTestDurationMs = 8000;
>   ^
>   static
> 
> and static constexpr yields a link errors:
> 
> Undefined symbols for architecture x86_64:
>   "webrtc::test::(anonymous namespace)::NoLossTest::kBytesSent", referenced
> from:
>       webrtc::test::(anonymous namespace)::NoLossTest::OnStreamsStopped() in
> audio_stats_test.o
> 
> I guess I'd have to redeclare the static constexpr outside the class
> declaration, but then simply using 'const' seemed less wordy...

Yeah, that. You could use the enum hack (which got much more powerful in C++11)
or bite the bullet and make declarations outside the class body, but this is
just a test class, so we don't really care if we duplicate these constants in
each instance. Go with just "const".

(C++17 will finally solve this problem, by the way.)

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/3008273002/diff/120001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:156: task_queue_.SendTask([this, test]() {
On 2017/09/13 14:53:33, the sun wrote:
> On 2017/09/13 14:20:26, kwiberg-webrtc wrote:
> > The lambda is capturing test by value, i.e. making a copy. Is that what you
> > want?
> 
> test is a pointer and CallTest owns the TQ, so this task should execute before
> the TEST_F defined test case is destroyed.

Acknowledged.

[the sun, 2017-09-14 06:34:15]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-09-14 06:34:16]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3008273002/#ps220001 (title:
"rebase")

[commit-bot: I haz the power, 2017-09-14 06:34:21]

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-09-14 07:14:09]

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

[commit-bot: I haz the power, 2017-09-14 07:14:11]

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

[the sun, 2017-09-14 08:09:39]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-09-14 08:09:39]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3008273002/#ps240001 (title:
"Make audio stats test optional on memcheck bots")

[commit-bot: I haz the power, 2017-09-14 08:09:50]

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-09-14 08:40:08]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1505376579168940,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"3d8c986a5bdef39f9aad11d956d7b89a146f6618"}

[commit-bot: I haz the power, 2017-09-14 08:40:10]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1505376579168940,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"3d8c986a5bdef39f9aad11d956d7b89a146f6618"}

[commit-bot: I haz the power, 2017-09-14 08:40:11]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1505376579168940,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"3d8c986a5bdef39f9aad11d956d7b89a146f6618"}

[commit-bot: I haz the power, 2017-09-14 08:40:12]

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

[commit-bot: I haz the power, 2017-09-14 08:40:14]

Failed to commit the patch.

[the sun, 2017-09-14 08:45:52]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-09-14 08:45:53]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3008273002/#ps260001 (title:
"rebase")

[commit-bot: I haz the power, 2017-09-14 08:46:01]

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-09-14 09:18:55]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1505378752857990,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"67c70d8ca17120afbaaf4269d13cfa3f6bd31415"}

[commit-bot: I haz the power, 2017-09-14 09:18:56]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1505378752857990,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"67c70d8ca17120afbaaf4269d13cfa3f6bd31415"}

[commit-bot: I haz the power, 2017-09-14 09:18:58]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1505378752857990,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"67c70d8ca17120afbaaf4269d13cfa3f6bd31415"}

[commit-bot: I haz the power, 2017-09-14 09:18:59]

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

[commit-bot: I haz the power, 2017-09-14 09:19:01]

Failed to commit the patch.

[the sun, 2017-09-14 12:13:29]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-09-14 12:13:29]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3008273002/#ps280001 (title:
"rebase")

[commit-bot: I haz the power, 2017-09-14 12:13:41]

CQ is trying da patch.

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

[the sun, 2017-09-14 12:46:53]

Description was changed from

==========
- Removes voe_conference_test.
- Adds a new AudioStatsTest, with better coverage of the same features, based on
call_test.
- Adds an AudioEndToEndTest utility, which AudioStatsTest and
LowBandwidthAudioTest uses.

BUG=webrtc:4690
==========

to

==========
- Removes voe_conference_test.
- Adds a new AudioStatsTest, with better coverage of the same features, based on
call_test.
- Adds an AudioEndToEndTest utility, which AudioStatsTest and
LowBandwidthAudioTest uses.

BUG=webrtc:4690
R=kwiberg@webrtc.org

Review-Url: https://codereview.webrtc.org/3008273002 .
Cr-Commit-Position: refs/heads/master@{#19833}
Committed:
https://webrtc.googlesource.com/src/+/73276ad7ed1e70ab764cd02d7189ed5839fadc20
==========

[the sun, 2017-09-14 12:46:55]

Committed patchset #15 (id:280001) manually as
73276ad7ed1e70ab764cd02d7189ed5839fadc20 (presubmit successful).