NetEq: Implement logging of Delayed Packet Outage Events

NetEq: Implement logging of Delayed Packet Outage Events

Measures the duration of each packet loss concealment (a.k.a. expand)
event that is not followed by a merge operation.

Having decoded and played packet m−1, the next expected packet is
m. If packet m arrives after some time of packet loss concealment, we
have a delayed packet outage event. However, if instead packet n>m
arrives, we have a lost packet outage event. In NetEq, the two outage
types results in different operations. Both types start with expand
operations to generate audio to play while the buffer is empty. When a
lost packet outage happens, the expand operation(s) are followed by
one merge operation. For delayed packet outages, merge is not done,
and the expand operations are immediately followed by normal
operations.

This change also includes unit tests for the new statistics.

BUG=webrtc:4915, chromium:488124
R=minyue@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/bef77e234fa53a52b830b5833948711f75ab8bbb

[hlundin-webrtc, 2015-08-13 13:33:41]

henrik.lundin@webrtc.org changed reviewers:
+ ivoc@webrtc.org, minyue@webrtc.org

[hlundin-webrtc, 2015-08-13 13:33:41]

henrik.lundin@webrtc.org changed required reviewers:
+ minyue@webrtc.org

[hlundin-webrtc, 2015-08-13 13:34:30]

Minyue, Ivo,

Please, take a look at this change.

Thanks!

[hlundin-webrtc, 2015-08-13 13:37:22]

The CQ bit was checked by henrik.lundin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-13 13:37:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1290113002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1290113002/1

[commit-bot: I haz the power, 2015-08-13 13:39:55]

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

[commit-bot: I haz the power, 2015-08-13 13:39:55]

Dry run: Try jobs failed on following builders:
  win on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win/builds/9090)
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/8971)

[hlundin-webrtc, 2015-08-13 13:45:45]

Fixing windows build

[minyue-webrtc, 2015-08-13 14:12:19]

On 2015/08/13 13:45:45, hlundin-webrtc wrote:
> Fixing windows build

Not sure if possible, but can we log at the receiving before decision making,
but not at action after decision making?

[hlundin-webrtc, 2015-08-14 07:25:04]

On 2015/08/13 14:12:19, minyue-webrtc wrote:
> On 2015/08/13 13:45:45, hlundin-webrtc wrote:
> > Fixing windows build
> 
> Not sure if possible, but can we log at the receiving before decision making,
> but not at action after decision making?

I don't think that is possible.

You can _guess_ that an outage will happen when the packets are inserted, but
the decision to do PLC to cover for a late/lost packet is not done until NetEq
really runs out of data to play. So, we cannot know that the outage event
happened until we have started calling Expand::Process. But, even then, we
cannot know if the outage was due to a late packet or a lost packet. That
knowledge will come as we decode the first packet _after_ the outage; if there
is a gap in the packet sequence, a loss happened, if not, it was a delay outage.

[hlundin-webrtc, 2015-08-14 07:33:08]

Fixing another Win(64) build error

[ivoc, 2015-08-14 13:00:03]

I only found a few minor nits to pick.

https://codereview.webrtc.org/1290113002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1290113002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/expand.cc:41: statistics_(statistics),
Shouldn't expand_duration_samples_ get an initial value as well in the
constructor?

https://codereview.webrtc.org/1290113002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand_unittest.cc (right):

https://codereview.webrtc.org/1290113002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand_unittest.cc:93:
rtc::scoped_ptr<int16_t[]> dummy_audio(new int16_t[speech_start_samples]);
Wouldn't a vector be easier here? (I feel like the vector police today)

[hlundin-webrtc, 2015-08-17 12:05:07]

Add new Move method and initialize a value

[hlundin-webrtc, 2015-08-17 12:07:28]

Addressing Ivo's comments.

https://codereview.webrtc.org/1290113002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1290113002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/expand.cc:41: statistics_(statistics),
On 2015/08/14 13:00:03, ivoc wrote:
> Shouldn't expand_duration_samples_ get an initial value as well in the
> constructor?

Done.

https://codereview.webrtc.org/1290113002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand_unittest.cc (right):

https://codereview.webrtc.org/1290113002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand_unittest.cc:93:
rtc::scoped_ptr<int16_t[]> dummy_audio(new int16_t[speech_start_samples]);
On 2015/08/14 13:00:03, ivoc wrote:
> Wouldn't a vector be easier here? (I feel like the vector police today)

I did the right thing and added a Move method to the file reader instead. More
for you to review... :)

[minyue-webrtc, 2015-08-17 14:07:22]

https://codereview.chromium.org/1290113002/diff/60001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.chromium.org/1290113002/diff/60001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1877: &stats_, fs_hz,
channels));
you share the pointer to stats_, how to prevent data race?

[hlundin-webrtc, 2015-08-17 14:10:31]

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1877: &stats_, fs_hz,
channels));
On 2015/08/17 14:07:22, minyue-webrtc wrote:
> you share the pointer to stats_, how to prevent data race?

All of NetEq is locked down with the same critsect. So, effectively, only one
thread at a time is running, and no call will be interrupted before complete.

[minyue-webrtc, 2015-08-17 14:16:08]

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1877: &stats_, fs_hz,
channels));
On 2015/08/17 14:10:31, hlundin-webrtc wrote:
> On 2015/08/17 14:07:22, minyue-webrtc wrote:
> > you share the pointer to stats_, how to prevent data race?
> 
> All of NetEq is locked down with the same critsect. So, effectively, only one
> thread at a time is running, and no call will be interrupted before complete.

Fair enough, although this type of sharing does not feel like a perfect
practice.

[hlundin-webrtc, 2015-08-17 14:23:01]

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1877: &stats_, fs_hz,
channels));
On 2015/08/17 14:16:08, minyue-webrtc wrote:
> On 2015/08/17 14:10:31, hlundin-webrtc wrote:
> > On 2015/08/17 14:07:22, minyue-webrtc wrote:
> > > you share the pointer to stats_, how to prevent data race?
> > 
> > All of NetEq is locked down with the same critsect. So, effectively, only
one
> > thread at a time is running, and no call will be interrupted before
complete.
> 
> Fair enough, although this type of sharing does not feel like a perfect
> practice.

I think this is an example of dependency injection, which we use in many places.

[minyue-webrtc, 2015-08-17 15:42:12]

some nits

https://codereview.chromium.org/1290113002/diff/60001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/neteq/expand_unittest.cc (right):

https://codereview.chromium.org/1290113002/diff/60001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/neteq/expand_unittest.cc:77: rtc::CheckedDivExact(2
* 2880 * test_sample_rate_hz_, 8000)),
why 2880? make a meaning name maybe

https://codereview.chromium.org/1290113002/diff/60001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/neteq/expand_unittest.cc:88: virtual void SetUp() {
probably
void SetUp() override

https://codereview.chromium.org/1290113002/diff/60001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/neteq/expand_unittest.cc:91: const size_t
speech_start_samples = test_sample_rate_hz_ * 1.1;
probably write conversion explicitly. I prefer

(size_t)(1.1f * test_sample_rate_hz_)

https://codereview.chromium.org/1290113002/diff/60001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/neteq/expand_unittest.cc:147: // TODO(hlundin):
Write more tests.
I'd like to see test on behavior after Reset().

[hlundin-webrtc, 2015-08-18 08:13:03]

Addressing Minyue's comments

[hlundin-webrtc, 2015-08-18 08:13:47]

PTAL again.

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand_unittest.cc (right):

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand_unittest.cc:77: rtc::CheckedDivExact(2
* 2880 * test_sample_rate_hz_, 8000)),
On 2015/08/17 15:42:11, minyue-webrtc wrote:
> why 2880? make a meaning name maybe

Done.

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand_unittest.cc:88: virtual void SetUp() {
On 2015/08/17 15:42:11, minyue-webrtc wrote:
> probably
> void SetUp() override

Done.

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand_unittest.cc:91: const size_t
speech_start_samples = test_sample_rate_hz_ * 1.1;
On 2015/08/17 15:42:11, minyue-webrtc wrote:
> probably write conversion explicitly. I prefer
> 
> (size_t)(1.1f * test_sample_rate_hz_)

Done.

https://codereview.webrtc.org/1290113002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand_unittest.cc:147: // TODO(hlundin):
Write more tests.
On 2015/08/17 15:42:11, minyue-webrtc wrote:
> I'd like to see test on behavior after Reset().

Done.

[minyue-webrtc, 2015-08-18 11:14:35]

LGTM + a comment

https://codereview.webrtc.org/1290113002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/input_audio_file.h (right):

https://codereview.webrtc.org/1290113002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/input_audio_file.h:41: virtual bool
Move(int samples);
I forgot to add to my earlier commments that I'd like this to be called Seek().
But you decide.

[hlundin-webrtc, 2015-08-18 11:50:22]

Rename InputAudioFile::Move to InputAudioFile::Seek

[hlundin-webrtc, 2015-08-18 11:50:37]

Thanks Minyue!

https://codereview.webrtc.org/1290113002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/input_audio_file.h (right):

https://codereview.webrtc.org/1290113002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/input_audio_file.h:41: virtual bool
Move(int samples);
On 2015/08/18 11:14:35, minyue-webrtc wrote:
> I forgot to add to my earlier commments that I'd like this to be called
Seek().
> But you decide. 

Done.

[hlundin-webrtc, 2015-08-18 11:50:47]

The CQ bit was checked by henrik.lundin@webrtc.org

[hlundin-webrtc, 2015-08-18 11:50:47]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1290113002/#ps100001 (title:
"Rename InputAudioFile::Move to InputAudioFile::Seek")

[commit-bot: I haz the power, 2015-08-18 11:50:55]

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

[commit-bot: I haz the power, 2015-08-18 11:52:26]

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

[commit-bot: I haz the power, 2015-08-18 11:52:27]

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

[hlundin-webrtc, 2015-08-18 12:07:22]

The CQ bit was checked by henrik.lundin@webrtc.org

[commit-bot: I haz the power, 2015-08-18 12:07:29]

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

[commit-bot: I haz the power, 2015-08-18 12:18:33]

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

[commit-bot: I haz the power, 2015-08-18 12:18:34]

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

[hlundin-webrtc, 2015-08-18 12:58:25]

Committed patchset #6 (id:100001) manually as
bef77e234fa53a52b830b5833948711f75ab8bbb (presubmit successful).