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

webrtc/modules/rtp_rtcp/source/rtp_sender.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 573 matching lines @@
                               uint32_t timestamp,
                               int64_t capture_time_ms,
                               int probe_cluster_id) {
   // Always send full padding packets. This is accounted for by the
   // RtpPacketSender,
   // which will make sure we don't send too much padding even if a single packet
   // is larger than requested.
   size_t padding_bytes_in_packet =
       std::min(MaxDataPayloadLength(), kMaxPaddingLength);
   size_t bytes_sent = 0;
-  bool using_transport_seq = rtp_header_extension_map_.IsRegistered(
-                                 kRtpExtensionTransportSequenceNumber) &&
-                             transport_sequence_number_allocator_;
+  bool using_transport_seq =
+      IsRtpHeaderExtensionRegistered(kRtpExtensionTransportSequenceNumber) &&
+      transport_sequence_number_allocator_;
   for (; bytes > 0; bytes -= padding_bytes_in_packet) {
     if (bytes < padding_bytes_in_packet)
       bytes = padding_bytes_in_packet;
 
     uint32_t ssrc;
     uint16_t sequence_number;
     int payload_type;
     bool over_rtx;
     {
       rtc::CritScope lock(&send_critsect_);
@@ skipping 33 matching lines @@
         }
         ssrc = ssrc_rtx_;
         sequence_number = sequence_number_rtx_;
         ++sequence_number_rtx_;
         payload_type = rtx_payload_type_map_.begin()->second;
         over_rtx = true;
       }
     }
 
     uint8_t padding_packet[IP_PACKET_SIZE];
-    size_t header_length =
-        CreateRtpHeader(padding_packet, payload_type, ssrc, false, timestamp,
-                        sequence_number, std::vector<uint32_t>());
+    size_t header_length = 0;
+    {
+      rtc::CritScope lock(&send_critsect_);
+      header_length =
+          CreateRtpHeader(padding_packet, payload_type, ssrc, false, timestamp,
+                          sequence_number, std::vector<uint32_t>());
+    }
     BuildPaddingPacket(padding_packet, header_length, padding_bytes_in_packet);
     size_t length = padding_bytes_in_packet + header_length;
     int64_t now_ms = clock_->TimeInMilliseconds();
 
     RtpUtility::RtpHeaderParser rtp_parser(padding_packet, length);
     RTPHeader rtp_header;
     rtp_parser.Parse(&rtp_header);
 
     if (capture_time_ms > 0) {
       UpdateTransmissionTimeOffset(
           padding_packet, length, rtp_header, now_ms - capture_time_ms);
     }
 
     UpdateAbsoluteSendTime(padding_packet, length, rtp_header, now_ms);
 
     PacketOptions options;
-    if (AllocateTransportSequenceNumber(&options.packet_id)) {
+    if (IsRtpHeaderExtensionRegistered(kRtpExtensionTransportSequenceNumber) &&
+        AllocateTransportSequenceNumber(&options.packet_id)) {
       if (UpdateTransportSequenceNumber(options.packet_id, padding_packet,
                                         length, rtp_header)) {
         if (transport_feedback_observer_)
           transport_feedback_observer_->AddPacket(options.packet_id, length,
                                                   probe_cluster_id);
       }
     }
 
     if (!SendPacketToNetwork(padding_packet, length, options))
       break;
@@ skipping 172 matching lines @@
     buffer_to_send_ptr = data_buffer_rtx;
   }
 
   int64_t now_ms = clock_->TimeInMilliseconds();
   int64_t diff_ms = now_ms - capture_time_ms;
   UpdateTransmissionTimeOffset(buffer_to_send_ptr, length, rtp_header,
                                diff_ms);
   UpdateAbsoluteSendTime(buffer_to_send_ptr, length, rtp_header, now_ms);
 
   PacketOptions options;
-  if (AllocateTransportSequenceNumber(&options.packet_id)) {
+  if (IsRtpHeaderExtensionRegistered(kRtpExtensionTransportSequenceNumber) &&
+      AllocateTransportSequenceNumber(&options.packet_id)) {
     if (UpdateTransportSequenceNumber(options.packet_id, buffer_to_send_ptr,
                                       length, rtp_header)) {
       if (transport_feedback_observer_)
         transport_feedback_observer_->AddPacket(options.packet_id, length,
                                                 probe_cluster_id);
     }
   }
 
   if (!is_retransmit && !send_over_rtx) {
     UpdateDelayStatistics(capture_time_ms, now_ms);
@@ skipping 112 matching lines @@
         corrected_time_ms > last_capture_time_ms_sent_) {
       last_capture_time_ms_sent_ = corrected_time_ms;
       TRACE_EVENT_ASYNC_BEGIN1(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                                "PacedSend", corrected_time_ms,
                                "capture_time_ms", corrected_time_ms);
     }
     return 0;
   }
 
   PacketOptions options;
-  if (AllocateTransportSequenceNumber(&options.packet_id)) {
+  if (IsRtpHeaderExtensionRegistered(kRtpExtensionTransportSequenceNumber) &&
+      AllocateTransportSequenceNumber(&options.packet_id)) {
     if (UpdateTransportSequenceNumber(options.packet_id, buffer, length,
                                       rtp_header)) {
       if (transport_feedback_observer_)
         transport_feedback_observer_->AddPacket(options.packet_id, length,
                                                 PacketInfo::kNotAProbe);
     }
   }
   UpdateDelayStatistics(capture_time_ms, now_ms);

[åsapersson, 2016/07/28 09:36:10]

The send delay statistics will now only be available if the extension is
registered. But maybe that is ok?

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

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.

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

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))?

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

That's a good idea. Fixed.

   UpdateOnSendPacket(options.packet_id, capture_time_ms, rtp_header.ssrc);
 
   bool sent = SendPacketToNetwork(buffer, length, options);
 
   // Mark the packet as sent in the history even if send failed. Dropping a
   // packet here should be treated as any other packet drop so we should be
   // ready for a retransmission.
   packet_history_.SetSent(rtp_header.sequenceNumber);
 
   if (!sent)
@@ skipping 870 matching lines @@
   rtc::CritScope lock(&send_critsect_);
 
   RtpState state;
   state.sequence_number = sequence_number_rtx_;
   state.start_timestamp = start_timestamp_;
 
   return state;
 }
 
 }  // namespace webrtc