Enable probing with repeated payload packets by default.

Enable probing with repeated payload packets by default.

To make this possible padding only packets will have the same timestamp
as the previously sent media packet, as long as RTX is not enabled. This
has the side effect that if we send only padding for a long time without
sending media, a receive-side jitter buffer could potentially overflow.

In practice this shouldn't be an issue, partly because RTX is recommended and
used by default, but also because padding typically is terminated before being
received by a client. It is also not an issue for bandwidth estimation as long
as abs-send-time is used instead of toffset.

BUG=chromium:425925
R=mflodman@webrtc.org, sprang@webrtc.org, tommi@webrtc.org

Committed: https://crrev.com/586b19bdb615dde34cdcf107272d8857fe2f5631
Cr-Commit-Position: refs/heads/master@{#9984}

[stefan-webrtc, 2015-09-03 10:50:16]

stefan@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[stefan-webrtc, 2015-09-03 10:52:31]

stefan@webrtc.org changed reviewers:
+ sprang@webrtc.org

[sprang_webrtc, 2015-09-03 12:13:55]

lgtm

[stefan-webrtc, 2015-09-08 12:13:10]

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

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

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

[mflodman, 2015-09-08 12:33:51]

https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
File talk/media/base/videoengine_unittest.h (right):

https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
talk/media/base/videoengine_unittest.h:1053: EXPECT_GT(NumRtpPackets(), 0);
Why this change?

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:574: size_t max_padding_length) {
This could be named only padding_length, since we'll use this and it's not
really a max that we might send less than.

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:575: size_t padding_bytes_in_packet
= max_padding_length;
No need for this variable.

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:587: return
padding_bytes_in_packet;
And no need to return the length here, it will always be = max_padding_length.

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:610: size_t padding_bytes_in_packet
= 0;
Won't this always be = std::min(MaxDataPayloadLength(), kMaxPaddingLength) and
never change?

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:616: // Always send full padding
packets.
Can you extend this comment to indicate this is take care of by the budget and
we're not sending too much over time.

[stefan-webrtc, 2015-09-08 13:17:35]

https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
File talk/media/base/videoengine_unittest.h (right):

https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
talk/media/base/videoengine_unittest.h:1053: EXPECT_GT(NumRtpPackets(), 0);
On 2015/09/08 12:33:50, mflodman wrote:
> Why this change?

It's no longer true that you only get 2 packets. The exact number of packet you
will get depends on how long the test has time to run before we have continued
to this line from line 1052. On my machine I'm getting 7 packets most of the
time, but some times 10 packets. It will be hard to have an EQ here when we're
doing probing.

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:574: size_t max_padding_length) {
On 2015/09/08 12:33:51, mflodman wrote:
> This could be named only padding_length, since we'll use this and it's not
> really a max that we might send less than.

Done.

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:575: size_t padding_bytes_in_packet
= max_padding_length;
On 2015/09/08 12:33:51, mflodman wrote:
> No need for this variable.

Done.

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:587: return
padding_bytes_in_packet;
On 2015/09/08 12:33:51, mflodman wrote:
> And no need to return the length here, it will always be = max_padding_length.

Done.

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:610: size_t padding_bytes_in_packet
= 0;
On 2015/09/08 12:33:51, mflodman wrote:
> Won't this always be = std::min(MaxDataPayloadLength(), kMaxPaddingLength) and
> never change?

Done.

https://codereview.webrtc.org/1327933003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:616: // Always send full padding
packets.
On 2015/09/08 12:33:51, mflodman wrote:
> Can you extend this comment to indicate this is take care of by the budget and
> we're not sending too much over time.

Done.

[stefan-webrtc, 2015-09-08 13:23:21]

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

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

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

[commit-bot: I haz the power, 2015-09-08 13:46:52]

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

[commit-bot: I haz the power, 2015-09-08 13:46:53]

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

[mflodman, 2015-09-08 18:32:03]

https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
File talk/media/base/videoengine_unittest.h (right):

https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
talk/media/base/videoengine_unittest.h:1053: EXPECT_GT(NumRtpPackets(), 0);
On 2015/09/08 13:17:34, stefan-webrtc (holmer) wrote:
> On 2015/09/08 12:33:50, mflodman wrote:
> > Why this change?
> 
> It's no longer true that you only get 2 packets. The exact number of packet
you
> will get depends on how long the test has time to run before we have continued
> to this line from line 1052. On my machine I'm getting 7 packets most of the
> time, but some times 10 packets. It will be hard to have an EQ here when we're
> doing probing.

It used to be '>= 2', not '==', and now it's '> 0', I was mainly curious about
lowering the number from '2' to '0'.

[stefan-webrtc, 2015-09-09 09:20:53]

https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
File talk/media/base/videoengine_unittest.h (right):

https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
talk/media/base/videoengine_unittest.h:1053: EXPECT_GT(NumRtpPackets(), 0);
On 2015/09/08 18:32:03, mflodman wrote:
> On 2015/09/08 13:17:34, stefan-webrtc (holmer) wrote:
> > On 2015/09/08 12:33:50, mflodman wrote:
> > > Why this change?
> > 
> > It's no longer true that you only get 2 packets. The exact number of packet
> you
> > will get depends on how long the test has time to run before we have
continued
> > to this line from line 1052. On my machine I'm getting 7 packets most of the
> > time, but some times 10 packets. It will be hard to have an EQ here when
we're
> > doing probing.
> 
> It used to be '>= 2', not '==', and now it's '> 0', I was mainly curious about
> lowering the number from '2' to '0'.

Oh, good point. I think I changed it by mistake. Only intended to change the one
below.

[stefan-webrtc, 2015-09-09 09:23:00]

https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
File talk/media/base/videoengine_unittest.h (right):

https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
talk/media/base/videoengine_unittest.h:1053: EXPECT_GT(NumRtpPackets(), 0);
On 2015/09/09 09:20:53, stefan-webrtc (holmer) wrote:
> On 2015/09/08 18:32:03, mflodman wrote:
> > On 2015/09/08 13:17:34, stefan-webrtc (holmer) wrote:
> > > On 2015/09/08 12:33:50, mflodman wrote:
> > > > Why this change?
> > > 
> > > It's no longer true that you only get 2 packets. The exact number of
packet
> > you
> > > will get depends on how long the test has time to run before we have
> continued
> > > to this line from line 1052. On my machine I'm getting 7 packets most of
the
> > > time, but some times 10 packets. It will be hard to have an EQ here when
> we're
> > > doing probing.
> > 
> > It used to be '>= 2', not '==', and now it's '> 0', I was mainly curious
about
> > lowering the number from '2' to '0'.
> 
> Oh, good point. I think I changed it by mistake. Only intended to change the
one
> below.

Sorry, I forgot why I change this. Notice that previously it checked for
NumRtpPackets() <= 2. Now I'm checking for NumRtpPackets() > 0.

[mflodman, 2015-09-09 10:14:34]

On 2015/09/09 09:23:00, stefan-webrtc (holmer) wrote:
>
https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
> File talk/media/base/videoengine_unittest.h (right):
> 
>
https://codereview.webrtc.org/1327933003/diff/60001/talk/media/base/videoengi...
> talk/media/base/videoengine_unittest.h:1053: EXPECT_GT(NumRtpPackets(), 0);
> On 2015/09/09 09:20:53, stefan-webrtc (holmer) wrote:
> > On 2015/09/08 18:32:03, mflodman wrote:
> > > On 2015/09/08 13:17:34, stefan-webrtc (holmer) wrote:
> > > > On 2015/09/08 12:33:50, mflodman wrote:
> > > > > Why this change?
> > > > 
> > > > It's no longer true that you only get 2 packets. The exact number of
> packet
> > > you
> > > > will get depends on how long the test has time to run before we have
> > continued
> > > > to this line from line 1052. On my machine I'm getting 7 packets most of
> the
> > > > time, but some times 10 packets. It will be hard to have an EQ here when
> > we're
> > > > doing probing.
> > > 
> > > It used to be '>= 2', not '==', and now it's '> 0', I was mainly curious
> about
> > > lowering the number from '2' to '0'.
> > 
> > Oh, good point. I think I changed it by mistake. Only intended to change the
> one
> > below.
> 
> Sorry, I forgot why I change this. Notice that previously it checked for
> NumRtpPackets() <= 2. Now I'm checking for NumRtpPackets() > 0.

Right, LGTM!

[stefan-webrtc, 2015-09-09 11:46:02]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-09-09 11:46:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1327933003/#ps80001 (title:
"Comments addressed.")

[commit-bot: I haz the power, 2015-09-09 11:46:09]

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

[commit-bot: I haz the power, 2015-09-09 11:48:22]

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

[commit-bot: I haz the power, 2015-09-09 11:48:22]

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

[stefan-webrtc, 2015-09-09 12:28:48]

stefan@webrtc.org changed reviewers:
+ tommi@webrtc.org

[stefan-webrtc, 2015-09-09 12:28:49]

+tommi for talk/media/base.

[tommi, 2015-09-09 13:04:15]

rs lgtm

[stefan-webrtc, 2015-09-10 11:35:39]

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

[commit-bot: I haz the power, 2015-09-10 11:35:49]

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

[stefan-webrtc, 2015-09-10 11:36:23]

Magnus, could you take another look at the latest patch-set? It solves some
issues I found when running ReceivesAndRetransmitsNack repeatedly, as it had
turned more flaky.

[stefan-webrtc, 2015-09-10 11:40:33]

https://codereview.webrtc.org/1327933003/diff/100001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/1327933003/diff/100001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/paced_sender.cc:364: if (!packets_->Empty())
No point in sending padding if we have media packets waiting to be sent.

https://codereview.webrtc.org/1327933003/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1327933003/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:620: }
Moving this in here solves the race I explained offline.

https://codereview.webrtc.org/1327933003/diff/100001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1327933003/diff/100001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:385: if (nacks_left_ <= 0 &&
It sometimes happened that we saw more nacks than 2, which made the test never
finish.

[mflodman, 2015-09-15 11:48:51]

One comment, then LGTM.

https://codereview.webrtc.org/1327933003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1327933003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:614: if (rtx_ != kRtxOff &&
last_timestamp_time_ms_ > 0) {
I'd like a comment explaining this.

[stefan-webrtc, 2015-09-15 12:01:58]

https://codereview.webrtc.org/1327933003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1327933003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:614: if (rtx_ != kRtxOff &&
last_timestamp_time_ms_ > 0) {
On 2015/09/15 11:48:51, mflodman wrote:
> I'd like a comment explaining this.

Done. I also  moved lines 614-619 down to the RTX section below (starting at
line 631).

[stefan-webrtc, 2015-09-15 12:02:41]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-09-15 12:02:41]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
sprang@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1327933003/#ps140001 (title:
"Comment addressed.")

[commit-bot: I haz the power, 2015-09-15 12:02:48]

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

[commit-bot: I haz the power, 2015-09-15 12:14:43]

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

[commit-bot: I haz the power, 2015-09-15 12:14:44]

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

[stefan-webrtc, 2015-09-15 12:48:24]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-09-15 12:48:24]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
sprang@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1327933003/#ps160001 (title:
"Changed threshold slightly to prevent a rare flake.")

[commit-bot: I haz the power, 2015-09-15 12:48:36]

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

[commit-bot: I haz the power, 2015-09-15 14:17:50]

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

[commit-bot: I haz the power, 2015-09-15 14:17:51]

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

[stefan-webrtc, 2015-09-16 15:41:59]

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

[commit-bot: I haz the power, 2015-09-16 15:42:09]

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

[stefan-webrtc, 2015-09-16 15:42:40]

I keep hitting flakes in libjingle. I hope this was the final one. Could you
take a quick look Magnus?

[commit-bot: I haz the power, 2015-09-16 17:42:19]

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

[commit-bot: I haz the power, 2015-09-16 17:42:20]

Dry run: Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_drmemory_light on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[mflodman, 2015-09-18 08:30:15]

still lgtm

[stefan-webrtc, 2015-09-18 08:30:33]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-09-18 08:30:33]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1327933003/#ps200001 (title:
"Fixing yet another flake in libjingle tests.")

[commit-bot: I haz the power, 2015-09-18 08:30:45]

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

[commit-bot: I haz the power, 2015-09-18 08:53:29]

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

[commit-bot: I haz the power, 2015-09-18 08:53:31]

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

[stefan-webrtc, 2015-09-18 09:14:48]

Committed patchset #11 (id:200001) manually as
586b19bdb615dde34cdcf107272d8857fe2f5631 (presubmit successful).

[commit-bot: I haz the power, 2015-09-18 09:14:50]

Patchset 11 (id:??) landed as
https://crrev.com/586b19bdb615dde34cdcf107272d8857fe2f5631
Cr-Commit-Position: refs/heads/master@{#9984}