Fix bug where transport sequence numbers are allocated for packets without the header extension reg…

Fix bug where transport sequence numbers are allocated for packets without the header extension registered.

This is an issue if the sequence numbers are to be used to compute packet loss statistics since it introduces gaps which are not related to loss.

Also making sure that the header extensions are properly guarded by the send crit sect.

Committed: https://crrev.com/a23fc626a2b6e8290ea96572a999802c88e5af61
Cr-Commit-Position: refs/heads/master@{#13557}

[stefan-webrtc, 2016-07-28 08:43:31]

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

[stefan-webrtc, 2016-07-28 08:44:13]

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

[commit-bot: I haz the power, 2016-07-28 08:44:17]

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, 2016-07-28 08:50:27]

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

[commit-bot: I haz the power, 2016-07-28 08:50:28]

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

[åsapersson, 2016-07-28 09:36:11]

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1010:
UpdateDelayStatistics(capture_time_ms, now_ms);
The send delay statistics will now only be available if the extension is
registered. But maybe that is ok?

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:520:
TEST_F(RtpSenderTestWithoutPacer, NoAllocationIfNotRegistered) {
add expect for no call to seq_num_allocator_, AllocateSequenceNumber()?

[stefan-webrtc, 2016-07-28 09:49:52]

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1010:
UpdateDelayStatistics(capture_time_ms, now_ms);
On 2016/07/28 09:36:10, åsapersson wrote:
> The send delay statistics will now only be available if the extension is
> registered. But maybe that is ok?

Good point... I think we'll have to live with that since currently the transport
sequence number is a bit broken. The goal is to always negotiate transport
sequence number for both audio and video, but we're not there yet.

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:520:
TEST_F(RtpSenderTestWithoutPacer, NoAllocationIfNotRegistered) {
On 2016/07/28 09:36:10, åsapersson wrote:
> add expect for no call to seq_num_allocator_, AllocateSequenceNumber()?

That's implicitly done by using a strict mock. Is there another way of expecting
no calls?

[åsapersson, 2016-07-28 10:44:50]

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1010:
UpdateDelayStatistics(capture_time_ms, now_ms);
On 2016/07/28 09:49:52, stefan-webrtc (holmer) wrote:
> On 2016/07/28 09:36:10, åsapersson wrote:
> > The send delay statistics will now only be available if the extension is
> > registered. But maybe that is ok?
> 
> Good point... I think we'll have to live with that since currently the
transport
> sequence number is a bit broken. The goal is to always negotiate transport
> sequence number for both audio and video, but we're not there yet.

Ok.

Maybe the allocation of the sequence number can be moved into
UpdateTransportSequenceNumber (which checks if the extension is registered (to
avoid taking lock twice))?

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:520:
TEST_F(RtpSenderTestWithoutPacer, NoAllocationIfNotRegistered) {
On 2016/07/28 09:49:52, stefan-webrtc (holmer) wrote:
> On 2016/07/28 09:36:10, åsapersson wrote:
> > add expect for no call to seq_num_allocator_, AllocateSequenceNumber()?
> 
> That's implicitly done by using a strict mock. Is there another way of
expecting
> no calls?

Ok.

[stefan-webrtc, 2016-07-28 12:35:02]

Updated tests.

[stefan-webrtc, 2016-07-28 12:54:28]

Comments addressed.

[stefan-webrtc, 2016-07-28 12:54:33]

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2190913002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1010:
UpdateDelayStatistics(capture_time_ms, now_ms);
On 2016/07/28 10:44:50, åsapersson wrote:
> On 2016/07/28 09:49:52, stefan-webrtc (holmer) wrote:
> > On 2016/07/28 09:36:10, åsapersson wrote:
> > > The send delay statistics will now only be available if the extension is
> > > registered. But maybe that is ok?
> > 
> > Good point... I think we'll have to live with that since currently the
> transport
> > sequence number is a bit broken. The goal is to always negotiate transport
> > sequence number for both audio and video, but we're not there yet.
> 
> Ok.
> 
> Maybe the allocation of the sequence number can be moved into
> UpdateTransportSequenceNumber (which checks if the extension is registered (to
> avoid taking lock twice))?

That's a good idea. Fixed.

[stefan-webrtc, 2016-07-28 12:54:43]

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

[commit-bot: I haz the power, 2016-07-28 12:54:58]

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

[åsapersson, 2016-07-28 13:19:00]

lgtm

[commit-bot: I haz the power, 2016-07-28 14:52:13]

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

[commit-bot: I haz the power, 2016-07-28 14:52:14]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-07-28 14:55:17]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2016-07-28 14:55:24]

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, 2016-07-28 14:56:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-28 14:56:53]

Description was changed from

==========
Fix bug where transport sequence numbers are allocated for packets without the
header extension registered.

This is an issue if the sequence numbers are to be used to compute packet loss
statistics since it introduces gaps which are not related to loss.

Also making sure that the header extensions are properly guarded by the send
crit sect.
==========

to

==========
Fix bug where transport sequence numbers are allocated for packets without the
header extension registered.

This is an issue if the sequence numbers are to be used to compute packet loss
statistics since it introduces gaps which are not related to loss.

Also making sure that the header extensions are properly guarded by the send
crit sect.

Committed: https://crrev.com/a23fc626a2b6e8290ea96572a999802c88e5af61
Cr-Commit-Position: refs/heads/master@{#13557}
==========

[commit-bot: I haz the power, 2016-07-28 14:56:54]

Patchset 3 (id:??) landed as
https://crrev.com/a23fc626a2b6e8290ea96572a999802c88e5af61
Cr-Commit-Position: refs/heads/master@{#13557}