Enable audio streams to send padding.

Enable audio streams to send padding.

Useful if bitrate probing is to be used with audio streams.

BUG=webrtc:7043
Review-Url: https://codereview.webrtc.org/2652893004
Cr-Commit-Position: refs/heads/master@{#16404}
Committed: https://chromium.googlesource.com/external/webrtc/+/e35f89a484ca376d5c187d166714eba578dfadc3

[stefan-webrtc, 2017-01-24 15:19:13]

Description was changed from

==========
Enable audio streams to send padding.

Useful if bitrate probing is to be used with audio streams.

BUG=webrtc:5079
==========

to

==========
Enable audio streams to send padding.

Useful if bitrate probing is to be used with audio streams.

BUG=webrtc:7043
==========

[stefan-webrtc, 2017-01-24 15:31:38]

Increase min padding length.

[stefan-webrtc, 2017-01-24 15:32:17]

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

[stefan-webrtc, 2017-01-24 15:34:02]

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

https://codereview.webrtc.org/2652893004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:509: if (!audio_configured_ &&
!last_packet_marker_bit_) {
Audio streams typically don't use the marker bit, and therefore it's not set.

[danilchap, 2017-01-24 15:53:14]

lgtm

[mflodman, 2017-01-26 14:03:37]

Is there any test to verify we can send audio with padding and we actually get
audio payload delivered with correct timestamps on the other side?

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

https://codereview.webrtc.org/2652893004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:485: size_t padding_bytes_in_packet
=
Can we do this if (!audio_configured_) {} else {} instead? I think that would be
easier to read.

https://codereview.webrtc.org/2652893004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:509: if (!audio_configured_ &&
!last_packet_marker_bit_) {
On 2017/01/24 15:34:02, stefan-webrtc wrote:
> Audio streams typically don't use the marker bit, and therefore it's not set.

Marker bits are used in the beginning of a talk spurt, so can you add something
to the comment above why we don't wait for a marker bit.

[stefan-webrtc, 2017-01-27 12:53:01]

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

https://codereview.webrtc.org/2652893004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:485: size_t padding_bytes_in_packet
=
On 2017/01/26 14:03:37, mflodman wrote:
> Can we do this if (!audio_configured_) {} else {} instead? I think that would
be
> easier to read.

Didn't become that much better, but it's hopefully a bit more readable.

https://codereview.webrtc.org/2652893004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:509: if (!audio_configured_ &&
!last_packet_marker_bit_) {
On 2017/01/26 14:03:37, mflodman wrote:
> On 2017/01/24 15:34:02, stefan-webrtc wrote:
> > Audio streams typically don't use the marker bit, and therefore it's not
set.
> 
> Marker bits are used in the beginning of a talk spurt, so can you add
something
> to the comment above why we don't wait for a marker bit.

Done.

[mflodman, 2017-01-27 13:03:39]

LGTM

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

https://codereview.webrtc.org/2652893004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:509: if (!audio_configured_ &&
!last_packet_marker_bit_) {
On 2017/01/27 12:53:01, stefan-webrtc wrote:
> On 2017/01/26 14:03:37, mflodman wrote:
> > On 2017/01/24 15:34:02, stefan-webrtc wrote:
> > > Audio streams typically don't use the marker bit, and therefore it's not
> set.
> > 
> > Marker bits are used in the beginning of a talk spurt, so can you add
> something
> > to the comment above why we don't wait for a marker bit.
> 
> Done.

Thanks!

[stefan-webrtc, 2017-01-27 13:06:27]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2017-01-27 13:06:28]

The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2652893004/#ps40001 (title:
"Fixed comments.")

[commit-bot: I haz the power, 2017-01-27 13:06:42]

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-01-27 13:34:14]

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

[commit-bot: I haz the power, 2017-01-27 13:34:15]

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

[stefan-webrtc, 2017-01-27 15:00:57]

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

[commit-bot: I haz the power, 2017-01-27 15:01:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[stefan-webrtc, 2017-01-27 15:17:47]

On 2017/01/26 14:03:37, mflodman wrote:
> Is there any test to verify we can send audio with padding and we actually get
> audio payload delivered with correct timestamps on the other side?

Could you elaborate on what your worries are? I think it might be possible to
write an end-to-end test, but ideally I hope it would be covered by a general
audio test (with bwe enabled), which verifies that the audio quality is ok.

[commit-bot: I haz the power, 2017-01-27 15:20:45]

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

[commit-bot: I haz the power, 2017-01-27 15:20:46]

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

[stefan-webrtc, 2017-02-01 16:02:51]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2017-02-01 16:02:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2652893004/#ps60001 (title:
"Fix issue where padding might be sent before the payload type is set.")

[commit-bot: I haz the power, 2017-02-01 16:02:58]

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-02-01 17:06:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485964971263600,
"parent_rev": "46fbb7d9d5b09556b78052839696731134ec7765", "commit_rev":
"e35f89a484ca376d5c187d166714eba578dfadc3"}

[commit-bot: I haz the power, 2017-02-01 17:06:33]

Description was changed from

==========
Enable audio streams to send padding.

Useful if bitrate probing is to be used with audio streams.

BUG=webrtc:7043
==========

to

==========
Enable audio streams to send padding.

Useful if bitrate probing is to be used with audio streams.

BUG=webrtc:7043

Review-Url: https://codereview.webrtc.org/2652893004
Cr-Commit-Position: refs/heads/master@{#16404}
Committed:
https://chromium.googlesource.com/external/webrtc/+/e35f89a484ca376d5c187d166...
==========

[commit-bot: I haz the power, 2017-02-01 17:06:34]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/e35f89a484ca376d5c187d166...

[Taylor Brandstetter, 2017-02-01 23:45:35]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.webrtc.org/2669033003/ by deadbeef@webrtc.org.

The reason for reverting is: Speculatively reverting, since Android end-to-end
tests (such as
https://build.chromium.org/p/client.webrtc/builders/Android64%20%28M%20Nexus5...)
started failing..