Add a flags field to video timing extension.

webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc

 /*
  *  Copyright (c) 2016 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 230 matching lines @@
 
 bool VideoContentTypeExtension::Write(uint8_t* data,
                                       VideoContentType content_type) {
   data[0] = static_cast<uint8_t>(content_type);
   return true;
 }
 
 // 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 bein an outlier.

[danilchap, 2017/08/11 12:22:35]

bein?

[sprang_webrtc, 2017/08/11 13:19:05]

typo, slightly rephrased.

+// 255 = Invalid. The whole timing frame extension should be ignored.

[danilchap, 2017/08/11 12:22:36]

what is a usecase to use this value on the wire?
if only used in code, may be change type to be wider (e.g. int instead of
uint8_t) and use value -1 as invalid.

Similar question for special value 0

[sprang_webrtc, 2017/08/11 13:19:05]

This value (255) should not normally be used on the wire, but can be so for
instance if a logging tool or content protection scheme etc assumes that the
headers for a set of packets have exactly the same layout. 

The 0 value is just intended to make it easy to have the same behavior as the
"old" wire format where there is no flag-field.

+//
 //    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 |

[danilchap, 2017/08/11 12:22:35]

len=12

[sprang_webrtc, 2017/08/11 13:19:05]

Nice catch.

 //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 //   | ms delta      |  packetizer finish ms delta     | pacer exit    |
 //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 //   | ms delta      |  network timestamp ms delta     | network2 time-|
 //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-//   | stamp ms delta|
-//   +-+-+-+-+-+-+-+-+
+//   | stamp ms delta|     flags     |
+//   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 
 constexpr RTPExtensionType VideoTimingExtension::kId;
 constexpr uint8_t VideoTimingExtension::kValueSizeBytes;
 constexpr const char VideoTimingExtension::kUri[];
 
 bool VideoTimingExtension::Parse(rtc::ArrayView<const uint8_t> data,
                                  VideoSendTiming* timing) {
   RTC_DCHECK(timing);
-  if (data.size() != kValueSizeBytes)
+  const bool is_old_protocol = data.size() == (kValueSizeBytes - 1);

[danilchap, 2017/08/11 12:22:35]

may be add TODO to remove support for old protocol in X months.

[sprang_webrtc, 2017/08/11 13:19:05]

Done.

+
+  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::kEncodeStartDeltaIdx);
   timing->encode_finish_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
-      data.data() + 2 * VideoSendTiming::kEncodeFinishDeltaIdx);
+      data.data() + VideoSendTiming::kEncodeFinishDeltaIdx);
   timing->packetization_finish_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
-      data.data() + 2 * VideoSendTiming::kPacketizationFinishDeltaIdx);
+      data.data() + VideoSendTiming::kPacketizationFinishDeltaIdx);
   timing->pacer_exit_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
-      data.data() + 2 * VideoSendTiming::kPacerExitDeltaIdx);
+      data.data() + VideoSendTiming::kPacerExitDeltaIdx);
   timing->network_timstamp_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
-      data.data() + 2 * VideoSendTiming::kNetworkTimestampDeltaIdx);
+      data.data() + VideoSendTiming::kNetworkTimestampDeltaIdx);
   timing->network2_timstamp_delta_ms = ByteReader<uint16_t>::ReadBigEndian(
-      data.data() + 2 * VideoSendTiming::kNetwork2TimestampDeltaIdx);
-  timing->is_timing_frame = true;
+      data.data() + VideoSendTiming::kNetwork2TimestampDeltaIdx);
+  timing->flags = is_old_protocol
+                      ? 0
+                      : ByteReader<uint8_t>::ReadBigEndian(
+                            data.data() + VideoSendTiming::kFLagsIdx);
   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::kEncodeStartDeltaIdx,
+      timing.encode_start_delta_ms);
+  ByteWriter<uint16_t>::WriteBigEndian(
+      data + VideoSendTiming::kEncodeFinishDeltaIdx,
       timing.encode_finish_delta_ms);
   ByteWriter<uint16_t>::WriteBigEndian(
-      data + 2 * VideoSendTiming::kPacketizationFinishDeltaIdx,
+      data + VideoSendTiming::kPacketizationFinishDeltaIdx,
       timing.packetization_finish_delta_ms);
   ByteWriter<uint16_t>::WriteBigEndian(
-      data + 2 * VideoSendTiming::kPacerExitDeltaIdx,
-      timing.pacer_exit_delta_ms);
+      data + VideoSendTiming::kPacerExitDeltaIdx, timing.pacer_exit_delta_ms);
   ByteWriter<uint16_t>::WriteBigEndian(
-      data + 2 * VideoSendTiming::kNetworkTimestampDeltaIdx, 0);  // reserved
+      data + VideoSendTiming::kNetworkTimestampDeltaIdx, 0);  // reserved
   ByteWriter<uint16_t>::WriteBigEndian(
-      data + 2 * VideoSendTiming::kNetwork2TimestampDeltaIdx, 0);  // reserved
+      data + VideoSendTiming::kNetwork2TimestampDeltaIdx, 0);  // reserved
+  ByteWriter<uint8_t>::WriteBigEndian(data + VideoSendTiming::kFLagsIdx,
+                                      timing.flags);
   return true;
 }
 
 bool VideoTimingExtension::Write(uint8_t* data,
                                  uint16_t time_delta_ms,
                                  uint8_t idx) {

[danilchap, 2017/08/11 12:22:35]

may be rename index to offset

[sprang_webrtc, 2017/08/11 13:19:05]

Done.

-  RTC_DCHECK_LT(idx, 6);
-  ByteWriter<uint16_t>::WriteBigEndian(data + 2 * idx, time_delta_ms);
+  RTC_DCHECK_LT(idx, kValueSizeBytes);

[danilchap, 2017/08/11 12:22:36]

may be LE(offset, kValueSizeBytes - 2)
since you write 2 bytes.

[sprang_webrtc, 2017/08/11 13:19:05]

Good idea. using kValueSizeBytes - sizeof(uint16_t) to make it even more clear.

+  ByteWriter<uint16_t>::WriteBigEndian(data + idx, time_delta_ms);
   return true;
 }
 
 bool BaseRtpStringExtension::Parse(rtc::ArrayView<const uint8_t> data,
                                    StringRtpHeaderExtension* str) {
   if (data.empty() || data[0] == 0)  // Valid string extension can't be empty.
     return false;
   str->Set(data);
   RTC_DCHECK(!str->empty());
   return true;
@@ skipping 31 matching lines @@
 constexpr RTPExtensionType RtpStreamId::kId;
 constexpr const char RtpStreamId::kUri[];
 
 constexpr RTPExtensionType RepairedRtpStreamId::kId;
 constexpr const char RepairedRtpStreamId::kUri[];
 
 constexpr RTPExtensionType RtpMid::kId;
 constexpr const char RtpMid::kUri[];
 
 }  // namespace webrtc