Add a flags field to video timing extension.

webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc
index 5e172c4f669c5caa0f081c9a7ef6d5525553f509..c4e8835d12376edf607a9c2fd38ec522c554f8e4 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc
@@ -248,18 +248,24 @@ bool VideoContentTypeExtension::Write(uint8_t* data,
 // Video Timing.
 // 6 timestamps in milliseconds counted from capture time stored in rtp header:
 // encode start/finish, packetization complete, pacer exit and reserved for
-// modification by the network modification.
+// modification by the network modification. |flags| is a bitmask and has the
+// following allowed values:
+// 0 = Valid data, but no flags available (backwards compatibility)
+// 1 = Frame marked as timing frame due to cyclic timer.
+// 2 = Frame marked as timing frame due to size being outside limit.
+// 255 = Invalid. The whole timing frame extension should be ignored.
+//
 //    0                   1                   2                   3
 //    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2
 //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-//   |  ID   | len=11|  encode start ms delta          | encode finish |
+//   |  ID   | len=12|  encode start ms delta          | encode finish |
 //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 //   | ms delta      |  packetizer finish ms delta     | pacer exit    |
 //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 //   | ms delta      |  network timestamp ms delta     | network2 time-|
 //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-//   | stamp ms delta|
-//   +-+-+-+-+-+-+-+-+
+//   | stamp ms delta|     flags     |

[danilchap, 2017/08/11 13:51:16]

did you consider putting flags first?
it would make this full comment nicer (times will be better aligned), will make
more logical adding more kind of timestamps later.

On minus side - will slightly complicate Parsing previous version.
(though that can be mitigated with an extra local variable:
const uint8* timestamps = data.data();
if (is_old_protocol) --timestamps;

old client will discard extension size != 11 anyway.

[sprang_webrtc, 2017/08/11 15:06:33]

I did, mainly abstained because of the Write method that takes a single field
and offset. But since we're always sending with the new format, let's go ahead
and change it.

Also, I think it's technically undefined behavior to move pointer outside your
memory area and then move it back again. But maybe we're ok since we actually
own the memory for the whole rtp header. Still, doing offset subtraction
instead.

+//   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 
 constexpr RTPExtensionType VideoTimingExtension::kId;
 constexpr uint8_t VideoTimingExtension::kValueSizeBytes;
@@ -268,47 +274,57 @@ constexpr const char VideoTimingExtension::kUri[];
 bool VideoTimingExtension::Parse(rtc::ArrayView<const uint8_t> data,
                                  VideoSendTiming* timing) {
   RTC_DCHECK(timing);
-  if (data.size() != kValueSizeBytes)
+  // TODO(sprang): Deprecate support for old wire format.
+  const bool is_old_protocol = data.size() == (kValueSizeBytes - 1);
+
+  if (!(data.size() == kValueSizeBytes || is_old_protocol))
     return false;
-  timing->encode_start_delta_ms =
-      ByteReader<uint16_t>::ReadBigEndian(data.data());
+  timing->encode_start_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
+      data.data() + VideoSendTiming::kEncodeStartDeltaOffset);
   timing->encode_finish_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
-      data.data() + 2 * VideoSendTiming::kEncodeFinishDeltaIdx);
+      data.data() + VideoSendTiming::kEncodeFinishDeltaOffset);
   timing->packetization_finish_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
-      data.data() + 2 * VideoSendTiming::kPacketizationFinishDeltaIdx);
+      data.data() + VideoSendTiming::kPacketizationFinishDeltaOffset);
   timing->pacer_exit_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
-      data.data() + 2 * VideoSendTiming::kPacerExitDeltaIdx);
+      data.data() + VideoSendTiming::kPacerExitDeltaOffset);
   timing->network_timstamp_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
-      data.data() + 2 * VideoSendTiming::kNetworkTimestampDeltaIdx);
+      data.data() + VideoSendTiming::kNetworkTimestampDeltaOffset);
   timing->network2_timstamp_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
-      data.data() + 2 * VideoSendTiming::kNetwork2TimestampDeltaIdx);
-  timing->is_timing_frame = true;
+      data.data() + VideoSendTiming::kNetwork2TimestampDeltaOffset);
+  timing->flags = is_old_protocol
+                      ? 0
+                      : ByteReader<uint8_t>::ReadBigEndian(
+                            data.data() + VideoSendTiming::kFLagsOffset);
   return true;
 }
 
 bool VideoTimingExtension::Write(uint8_t* data, const VideoSendTiming& timing) {
-  ByteWriter<uint16_t>::WriteBigEndian(data, timing.encode_start_delta_ms);
   ByteWriter<uint16_t>::WriteBigEndian(
-      data + 2 * VideoSendTiming::kEncodeFinishDeltaIdx,
+      data + VideoSendTiming::kEncodeStartDeltaOffset,
+      timing.encode_start_delta_ms);
+  ByteWriter<uint16_t>::WriteBigEndian(
+      data + VideoSendTiming::kEncodeFinishDeltaOffset,
       timing.encode_finish_delta_ms);
   ByteWriter<uint16_t>::WriteBigEndian(
-      data + 2 * VideoSendTiming::kPacketizationFinishDeltaIdx,
+      data + VideoSendTiming::kPacketizationFinishDeltaOffset,
       timing.packetization_finish_delta_ms);
   ByteWriter<uint16_t>::WriteBigEndian(
-      data + 2 * VideoSendTiming::kPacerExitDeltaIdx,
+      data + VideoSendTiming::kPacerExitDeltaOffset,
       timing.pacer_exit_delta_ms);
   ByteWriter<uint16_t>::WriteBigEndian(
-      data + 2 * VideoSendTiming::kNetworkTimestampDeltaIdx, 0);  // reserved
+      data + VideoSendTiming::kNetworkTimestampDeltaOffset, 0);  // reserved
   ByteWriter<uint16_t>::WriteBigEndian(
-      data + 2 * VideoSendTiming::kNetwork2TimestampDeltaIdx, 0);  // reserved
+      data + VideoSendTiming::kNetwork2TimestampDeltaOffset, 0);  // reserved
+  ByteWriter<uint8_t>::WriteBigEndian(data + VideoSendTiming::kFLagsOffset,
+                                      timing.flags);
   return true;
 }
 
 bool VideoTimingExtension::Write(uint8_t* data,
                                  uint16_t time_delta_ms,
-                                 uint8_t idx) {
-  RTC_DCHECK_LT(idx, 6);
-  ByteWriter<uint16_t>::WriteBigEndian(data + 2 * idx, time_delta_ms);
+                                 uint8_t offset) {
+  RTC_DCHECK_LT(offset, kValueSizeBytes - sizeof(uint16_t));
+  ByteWriter<uint16_t>::WriteBigEndian(data + offset, time_delta_ms);
   return true;
 }