Implement FrameMarking header extension support

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 3f4c401a19d5c43ff9f64c6a9e3bd14d60ff9fb6..7321edc97413fd52887a9f1a7a8073c117fbf71f 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -301,6 +301,56 @@ 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);
+
+  // Set Frame Marks

[danilchap, 2017/06/28 16:07:07]

Prefer to end comments with a '.'

[sergio.garcia.murillo, 2017/06/29 12:58:58]

Acknowledged.

+  FrameMarks frame_marks;
+  bool frame_marking_enabled = true;
+
+  // Common info
+  frame_marks.startOfFrame = true;
+  frame_marks.endOfFrame = false;
+  frame_marks.independent = (frame_type == kVideoFrameKey);
+
+  // Codec specific
+  switch (video_type) {
+    case kRtpVideoH264:
+      // Nothing to add
+      frame_marks.discardable = false;
+      frame_marks.temporalLayerId = 0;

[danilchap, 2017/06/28 16:07:07]

may be kNoTemporalIdx/kNoSpaitalIdx/kNoTl0PicIdx?

[sergio.garcia.murillo, 2017/06/29 12:58:58]

Done.

+      frame_marks.spatialLayerId = 0;
+      frame_marks.tl0PicIdx = 0;
+      break;
+    case kRtpVideoVp8:
+      frame_marks.discardable = video_header->codecHeader.VP8.nonReference;
+      frame_marks.baseLayerSync = video_header->codecHeader.VP8.layerSync;
+      frame_marks.temporalLayerId = video_header->codecHeader.VP8.temporalIdx;
+      frame_marks.spatialLayerId = 0;

[danilchap, 2017/06/28 16:07:07]

kNoSpatialIdx?

[sergio.garcia.murillo, 2017/06/29 12:58:58]

Done.

+      frame_marks.tl0PicIdx = video_header->codecHeader.VP8.tl0PicIdx;
+      break;
+    case kRtpVideoVp9:
+      frame_marks.discardable = false;
+      frame_marks.temporalLayerId = video_header->codecHeader.VP9.temporal_idx;
+      frame_marks.spatialLayerId = 0;
+      frame_marks.tl0PicIdx = video_header->codecHeader.VP9.tl0_pic_idx;
+      // TODO: This will need to be changed to support VP9 SVC, but videoheader

[danilchap, 2017/06/28 16:07:07]

add bugnumber or username to TODO()
https://google.github.io/styleguide/cppguide.html#TODO_Comments

+      // is set per-frame, not per packet, so we can't have access to this

[danilchap, 2017/06/28 16:07:06]

this is same for VP8, why is it a problem? SendVideo called per layer frame, not
per super frame for VP9.
same as with VP8 all fields except start_of_frame/end_of_frame can be set in
advance, those two fields need modification per-packet anyway.

[sergio.garcia.murillo, 2017/06/29 12:58:58]

No, because in VP8 there is no support for spatial scalability, so all packets
in the frame have same spatial, temporal and tl0picid. 

On VP9 the spatial layer id, changes between packets on the same frame.

[danilchap, 2017/06/29 13:47:26]

SendVideo takes layer frame, not super frame for VP9, i.e. all packets here use
same spatial layer id.

[sergio.garcia.murillo, 2017/06/30 10:09:23]

Perfect then.

Side note, wouldn't it still be a problem with:
     codecHeader.VP9.beginning_of_frame
and 
     codecHeader.VP9.end_of_frame values

They are per packet and not per layer frame (although not relevant for this CL)

[danilchap, 2017/06/30 12:53:25]

same struct is used for per-packet vp9 header (when parsed).
These two fields expected to be unused in this function.

+      // values.
+      // Also, small modifications to the extensions will be needed to not
+      // change size of the extension between sid:0 and sid:1
+//    frame_marks.startOfFrame =
+//        video_header->codecHeader.VP9.beginning_of_frame;
+//    frame_marks.endOfFrame = video_header->codecHeader.VP9.end_of_frame;
+//    frame_marks.spatialLayerId = video_header->codecHeader.VP9.spatial_idx;
+      break;
+    default:
+      // Do not use frame marking
+      frame_marking_enabled = false;
+  }
+  // Only add frame marking for known codecs
+  if (frame_marking_enabled)
+      // Add extension header for frame marking
+      rtp_header->SetExtension<FrameMarking>(frame_marks);
+
   auto last_packet = rtc::MakeUnique<RtpPacketToSend>(*rtp_header);
 
   size_t fec_packet_overhead;
@@ -377,6 +427,7 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
 
   bool first_frame = first_frame_sent_();
   for (size_t i = 0; i < num_packets; ++i) {
+    bool first = (i == 0);
     bool last = (i + 1) == num_packets;
     auto packet = last ? std::move(last_packet)
                        : rtc::MakeUnique<RtpPacketToSend>(*rtp_header);
@@ -385,6 +436,16 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
     RTC_DCHECK_LE(packet->payload_size(),
                   last ? max_data_payload_length - last_packet_reduction_len
                        : max_data_payload_length);
+
+    // Update start and end marks

[danilchap, 2017/06/28 16:07:07]

may be put this block inside if (frame_marking_enabled) 

[sergio.garcia.murillo, 2017/06/29 12:58:58]

Done.

+    frame_marks.startOfFrame = first;
+    frame_marks.endOfFrame = last;
+
+    // Only add frame marking for known codecs
+    if (frame_marking_enabled)
+      // Update extension header for frame marking
+      packet->SetExtension<FrameMarking>(frame_marks);
+
     if (!rtp_sender_->AssignSequenceNumber(packet.get()))
       return false;