Reland of Enable audio streams to send padding. (patchset #4 id:60001 of https://codereview.webrtc.…

Reland of Enable audio streams to send padding. (patchset #4 id:60001 of https://codereview.webrtc.org/2652893004/ )

Original issue's description:
> 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

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

[stefan-webrtc, 2017-02-02 15:59:13]

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

[commit-bot: I haz the power, 2017-02-02 15:59:23]

Dry run: 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-02 16:08:32]

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

[commit-bot: I haz the power, 2017-02-02 16:08:33]

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

[stefan-webrtc, 2017-02-03 08:22:32]

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

[commit-bot: I haz the power, 2017-02-03 08:22:38]

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-02-03 08:42:14]

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

[stefan-webrtc, 2017-02-03 08:42:15]

PTAL. I believe I've fixed the flakiness that caused this to be reverted by
ensuring that we only send padding on streams with BWE extensions. Sending
padding on streams without BWE extensions is pointless.

[commit-bot: I haz the power, 2017-02-03 09:14:39]

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

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

Dry run: This issue passed the CQ dry run.

[terelius, 2017-02-03 14:26:15]

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

https://codereview.webrtc.org/2675703002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:489: std::max(std::min(bytes,
MaxPayloadSize()), kMinAudioPaddingLength);
Note this order of operation allows sending padding packets that are  larger
than MaxPayloadSize(). It this the intent? Or do we assume that
kMinAudioPaddingLength < MaxPayloadSize()?

https://codereview.webrtc.org/2675703002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:518: // For audio marker bits
doesn't mark the end of a frame and frames
What does the marker bit mean for audio? Though unlikely, what happens if we add
padding in the middle of an audio frame? Can it corrupt the decoder state?

[stefan-webrtc, 2017-02-03 14:39:16]

Comments addressed.

[stefan-webrtc, 2017-02-03 14:39:26]

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

https://codereview.webrtc.org/2675703002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:489: std::max(std::min(bytes,
MaxPayloadSize()), kMinAudioPaddingLength);
On 2017/02/03 14:26:14, terelius wrote:
> Note this order of operation allows sending padding packets that are  larger
> than MaxPayloadSize(). It this the intent? Or do we assume that
> kMinAudioPaddingLength < MaxPayloadSize()?

Thanks, that's not the intent.

https://codereview.webrtc.org/2675703002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:518: // For audio marker bits
doesn't mark the end of a frame and frames
On 2017/02/03 14:26:14, terelius wrote:
> What does the marker bit mean for audio? Though unlikely, what happens if we
add
> padding in the middle of an audio frame? Can it corrupt the decoder state?

It has no clear meaning, but one possible interpretation is "the beginning of a
talk spurt"
http://www.cs.columbia.edu/~hgs/rtp/faq.html#marker

It should not affect the decoder state. I don't even think padding packets are
passed down to the decoder. They don't contain anything, so they won't add bits
to the bitstream.

It's a bit worse for video because we have to keep track of sequence numbers in
a more complicated way, and we also need to know when a frame starts and ends.
In theory we could remove this check also for video, but it would require us to
nack some padding packets.

[terelius, 2017-02-03 14:42:19]

lgtm

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

https://codereview.webrtc.org/2675703002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:518: // For audio marker bits
doesn't mark the end of a frame and frames
On 2017/02/03 14:39:25, stefan-webrtc wrote:
> On 2017/02/03 14:26:14, terelius wrote:
> > What does the marker bit mean for audio? Though unlikely, what happens if we
> add
> > padding in the middle of an audio frame? Can it corrupt the decoder state?
> 
> It has no clear meaning, but one possible interpretation is "the beginning of
a
> talk spurt"
> http://www.cs.columbia.edu/~hgs/rtp/faq.html#marker
> 
> It should not affect the decoder state. I don't even think padding packets are
> passed down to the decoder. They don't contain anything, so they won't add
bits
> to the bitstream.
> 
> It's a bit worse for video because we have to keep track of sequence numbers
in
> a more complicated way, and we also need to know when a frame starts and ends.
> In theory we could remove this check also for video, but it would require us
to
> nack some padding packets.

Acknowledged.

[stefan-webrtc, 2017-02-03 14:53:20]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2017-02-03 14:53:30]

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-03 16:13:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486133600828290,
"parent_rev": "b11fb25c1230b2ef1085b9c0aa0642d6494afa88", "commit_rev":
"53b6cc3832f72db8d7aaf293723f8d2d1bef757d"}

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

Description was changed from

==========
Reland of Enable audio streams to send padding. (patchset #4 id:60001 of
https://codereview.webrtc.org/2652893004/ )

Original issue's description:
> 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...

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

to

==========
Reland of Enable audio streams to send padding. (patchset #4 id:60001 of
https://codereview.webrtc.org/2652893004/ )

Original issue's description:
> 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...

BUG=webrtc:7043

Review-Url: https://codereview.webrtc.org/2675703002
Cr-Commit-Position: refs/heads/master@{#16433}
Committed:
https://chromium.googlesource.com/external/webrtc/+/53b6cc3832f72db8d7aaf2937...
==========

[commit-bot: I haz the power, 2017-02-03 16:14:02]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/53b6cc3832f72db8d7aaf2937...