Fix packetization logic to leave space for extensions in the last packet

webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
index b89aefef53e44977f2bda96438bd16cb3b4572af..aeb72bb5907f859139277d61f47694cae642e05d 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -19,6 +19,7 @@
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
+#include "webrtc/base/ptr_util.h"
 #include "webrtc/base/trace_event.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "webrtc/modules/rtp_rtcp/source/byte_io.h"
@@ -300,6 +301,7 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
   rtp_header->SetPayloadType(payload_type);
   rtp_header->SetTimestamp(rtp_timestamp);
   rtp_header->set_capture_time_ms(capture_time_ms);
+  auto last_packet = rtc::MakeUnique<RtpPacketToSend>(*rtp_header);
 
   size_t fec_packet_overhead;
   bool red_enabled;
@@ -322,12 +324,12 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
       VideoRotation current_rotation = video_header->rotation;
       if (frame_type == kVideoFrameKey || current_rotation != last_rotation_ ||
           current_rotation != kVideoRotation_0)
-        rtp_header->SetExtension<VideoOrientation>(current_rotation);
+        last_packet->SetExtension<VideoOrientation>(current_rotation);
       last_rotation_ = current_rotation;
       // Report content type only for key frames.
       if (frame_type == kVideoFrameKey &&
           video_header->content_type != VideoContentType::UNSPECIFIED) {
-        rtp_header->SetExtension<VideoContentTypeExtension>(
+        last_packet->SetExtension<VideoContentTypeExtension>(
             video_header->content_type);
       }
     }
@@ -350,10 +352,14 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
                            (rtp_sender_->RtxStatus() ? kRtxHeaderSize : 0);
   RTC_DCHECK_LE(packet_capacity, rtp_header->capacity());
   RTC_DCHECK_GT(packet_capacity, rtp_header->headers_size());
+  RTC_DCHECK_GT(packet_capacity, last_packet->headers_size());
   size_t max_data_payload_length = packet_capacity - rtp_header->headers_size();
+  RTC_DCHECK_GE(last_packet->headers_size(), rtp_header->headers_size());
+  size_t last_packet_extensions_len =

[danilchap, 2017/05/12 18:56:15]

this variable name make false impression there are no extensions in the
rtp_header. 
wouldn't mind your suggestion from the interface:
last_packet_reduction_len

[ilnik, 2017/05/15 09:38:33]

Done.

+      last_packet->headers_size() - rtp_header->headers_size();
 
   std::unique_ptr<RtpPacketizer> packetizer(RtpPacketizer::Create(
-      video_type, max_data_payload_length,
+      video_type, max_data_payload_length, last_packet_extensions_len,
       video_header ? &(video_header->codecHeader) : nullptr, frame_type));
   // Media packet storage.
   StorageType storage = packetizer->GetStorageType(retransmission_settings);
@@ -363,18 +369,18 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
   // issue is fixed.
   const RTPFragmentationHeader* frag =
       (video_type == kRtpVideoVp8) ? nullptr : fragmentation;
-  packetizer->SetPayloadData(payload_data, payload_size, frag);
+  size_t num_packets =
+      packetizer->SetPayloadData(payload_data, payload_size, frag);
 

[danilchap, 2017/05/12 18:56:15]

what happen if num_packets is zero?

[ilnik, 2017/05/15 09:38:34]

Like a previous implementation, we should return false. Done now.

   bool first_frame = first_frame_sent_();
-  bool first = true;
-  bool last = false;
-  while (!last) {
-    std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(*rtp_header));
-
-    if (!packetizer->NextPacket(packet.get(), &last))
+  for (size_t i = 0; i < num_packets; ++i) {
+    bool last = (i + 1) == num_packets;
+    auto packet = last ? std::move(last_packet)
+                       : rtc::MakeUnique<RtpPacketToSend>(*rtp_header);
+    if (!packetizer->NextPacket(packet.get()))
       return false;
-    RTC_DCHECK_LE(packet->payload_size(), max_data_payload_length);
-
+    RTC_DCHECK_LE(packet->payload_size(),
+                  packet->capacity() - packet->headers_size());

[danilchap, 2017/05/12 18:56:15]

do not use packet->capacity() here, it has different meaning (it is buffer
capacity, while packet_capacity is mtu - transport headers - rtx/fec overhead)
feel free to rename variable packet_capacity to max_rtp_packet_size or similar
to avoid confusion.

But you may use fact
packet->header_size() + packet->payload_size() == packet->size().
(technically header_size + payload_size + padding_size == size, but padding_size
always zero here and can be ignored)

[ilnik, 2017/05/15 09:38:34]

Redone this check now.

     if (!rtp_sender_->AssignSequenceNumber(packet.get()))
       return false;
 
@@ -392,7 +398,7 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
     }
 
     if (first_frame) {
-      if (first) {
+      if (i == 0) {
         LOG(LS_INFO)
             << "Sent first RTP packet of the first video frame (pre-pacer)";
       }
@@ -401,7 +407,6 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
             << "Sent last RTP packet of the first video frame (pre-pacer)";
       }
     }
-    first = false;
   }
 
   TRACE_EVENT_ASYNC_END1("webrtc", "Video", capture_time_ms, "timestamp",