New jitter buffer experiment.

webrtc/modules/video_coding/rtp_frame_reference_finder.cc

Index: webrtc/modules/video_coding/rtp_frame_reference_finder.cc
diff --git a/webrtc/modules/video_coding/rtp_frame_reference_finder.cc b/webrtc/modules/video_coding/rtp_frame_reference_finder.cc
index 861a1178c006336bdb4517dbb8f24296623bf5c7..9ba60380c8d682ab2d92ad81aeebb52cae4ce147 100644
--- a/webrtc/modules/video_coding/rtp_frame_reference_finder.cc
+++ b/webrtc/modules/video_coding/rtp_frame_reference_finder.cc
@@ -371,6 +371,11 @@ void RtpFrameReferenceFinder::ManageFrameVp9(
 
   const RTPVideoHeaderVP9& codec_header = rtp_codec_header->VP9;
 
+  bool old_frame = Vp9PidTl0Fix(*frame, &rtp_codec_header->VP9.picture_id,
+                                &rtp_codec_header->VP9.tl0_pic_idx);
+  if (old_frame)
+    return;
+
   if (codec_header.picture_id == kNoPictureId ||
       codec_header.temporal_idx == kNoTemporalIdx) {
     ManageFrameGeneric(std::move(frame), codec_header.picture_id);
@@ -585,5 +590,126 @@ uint16_t RtpFrameReferenceFinder::UnwrapPictureId(uint16_t picture_id) {
   return last_unwrap_;
 }
 
+bool RtpFrameReferenceFinder::Vp9PidTl0Fix(const RtpFrameObject& frame,

[stefan-webrtc, 2016/11/14 12:00:26]

I think we'll have to add tests to make sure these cases work as intended... :)

[philipel, 2016/11/14 15:31:04]

Done.

+                                           int16_t* picture_id,
+                                           int16_t* tl0_pic_idx) {
+  // We are currently receiving VP9 without PID, nothing to fix.
+  if (*picture_id == kNoPictureId)
+    return false;
+
+  // If |vp9_fix_jump_timestamp_| != -1 then a jump has occurred recently.
+  if (vp9_fix_jump_timestamp_ != -1) {
+    // If this frame has a timestamp older than |vp9_fix_jump_timestamp_| then
+    // this frame is old (more previous than the frame where we detected the
+    // jump) and should be dropped.
+    if (AheadOf<uint32_t>(vp9_fix_jump_timestamp_, frame.timestamp))
+      return true;
+
+    // After 60 seconds, reset |vp9_fix_jump_timestamp_| in order to not
+    // discard old frames when the timestamp wraps.
+    int diff_ms =
+        ForwardDiff<uint32_t>(vp9_fix_jump_timestamp_, frame.timestamp) / 90;
+    if (diff_ms > 60 * 1000)
+      vp9_fix_jump_timestamp_ = -1;

[stefan-webrtc, 2016/11/14 12:00:26]

Should vp9_fix_last_picture_id_ be reset here too?

[philipel, 2016/11/14 15:31:03]

Not needed since |vp9_fix_last_picture_id_| is continuously updated so it will
never become old.

+  }
+
+  uint16_t fixed_pid = Add<kPicIdLength>(*picture_id, vp9_fix_pid_offset_);
+
+  // Update |vp9_fix_last_timestamp_| with the most recent timestamp.
+  if (vp9_fix_last_timestamp_ == -1)
+    vp9_fix_last_timestamp_ = frame.timestamp;
+  if (AheadOf<uint32_t>(frame.timestamp, vp9_fix_last_timestamp_))
+    vp9_fix_last_timestamp_ = frame.timestamp;
+
+  if (vp9_fix_last_picture_id_ == -1)
+    vp9_fix_last_picture_id_ = *picture_id;
+
+  bool has_jumped = false;
+
+  if (AheadOrAt<uint32_t>(frame.timestamp, vp9_fix_last_timestamp_) &&

[stefan-webrtc, 2016/11/14 12:00:26]

I think it could have been good to split this method into three different:
DetectPicIdJump()
DetectTl0PicIdxJump()
CorrectForJump()

WDYT?

[philipel, 2016/11/14 15:31:03]

Done, but not CorrectForJump (annoying to return multiple recalculated values).

+      AheadOf<uint16_t, kPicIdLength>(vp9_fix_last_picture_id_, fixed_pid)) {
+    // Test if there has been a jump backwards in the picture id.
+    has_jumped = true;
+  } else if (AheadOrAt<uint32_t>(frame.timestamp, vp9_fix_last_timestamp_) &&
+             ForwardDiff<uint16_t, kPicIdLength>(vp9_fix_last_picture_id_,
+                                                 fixed_pid) > 128) {
+    // Test if we have jump forward too much. The reason we have to do this

[stefan-webrtc, 2016/11/14 12:00:26]

jumped forward

[philipel, 2016/11/14 15:31:03]

Done.

+    // is because the FrameBuffer holds history of old frames and inserting
+    // frames with a much advanced picture id can result in the frame buffer
+    // holding more than half of the interval of picture ids.
+    has_jumped = true;
+  } else {
+    // Special case where the picture id jump forward but not by much and the
+    // tl0 jumps to the id of an already saved gof for that id. In order to
+    // detect this we check if the picture id span over the length of the GOF.
+    auto info_it = gof_info_.find(*tl0_pic_idx);
+    if (info_it != gof_info_.end()) {
+      int last_pid_gof_idx_0 =
+          Subtract<kPicIdLength>(info_it->second.last_picture_id,
+                                 info_it->second.last_picture_id %
+                                     info_it->second.gof->num_frames_in_gof);
+      int pif_gof_end = Add<kPicIdLength>(
+          last_pid_gof_idx_0, info_it->second.gof->num_frames_in_gof);
+      if (AheadOf<uint16_t, kPicIdLength>(fixed_pid, pif_gof_end))
+        has_jumped = true;
+    }
+  }
+
+  uint8_t fixed_tl0 = kNoTl0PicIdx;
+  if (*tl0_pic_idx != kNoTl0PicIdx) {
+    fixed_tl0 = Add<256>(*tl0_pic_idx, vp9_fix_tl0_pic_idx_offset_);

[stefan-webrtc, 2016/11/14 12:00:26]

Should we have a named constant 256?

[philipel, 2016/11/14 15:31:03]

Done.

+
+    // Update |vp9_fix_last_tl0_pic_idx_| with the most recent tl0 pic index.
+    if (vp9_fix_last_tl0_pic_idx_ == -1)
+      vp9_fix_last_tl0_pic_idx_ = *tl0_pic_idx;
+    if (AheadOf<uint8_t>(fixed_tl0, vp9_fix_last_tl0_pic_idx_))
+      vp9_fix_last_tl0_pic_idx_ = fixed_tl0;

[stefan-webrtc, 2016/11/14 12:00:26]

Should we break out these 4 lines into a function that can also be used at lines
618-622? Not sure if it's feasible... :)

[philipel, 2016/11/14 15:31:03]

Not really feasible...

+
+    // Test if there has been a jump backwards in tl0 pic index.

[stefan-webrtc, 2016/11/14 12:00:26]

Can there be a jump in tl0 pic index but not in picture id, or is it enough to
track picture id jumps?

[philipel, 2016/11/14 15:31:03]

I guess picture id might be enough but I think it's better if we track both.

+    if (AheadOrAt<uint32_t>(frame.timestamp, vp9_fix_last_timestamp_) &&
+        AheadOf<uint8_t>(vp9_fix_last_tl0_pic_idx_, fixed_tl0)) {
+      has_jumped = true;
+    }
+
+    // Test if there has been a jump forward. If the jump forward results
+    // in the tl0 pic index for this frame to be considered smaller than the
+    // smallest item in |gof_info_| then we have jumped forward far enough to
+    // wrap.
+    if (!gof_info_.empty() &&
+        AheadOf<uint8_t>(gof_info_.begin()->first, fixed_tl0)) {
+      has_jumped = true;
+    }
+  }
+
+  if (has_jumped) {
+    // First we calculate the offset to get to the previous picture id, and then
+    // we add 128 (max pid_diff) to avoid accidently referencing any previous
+    // frames that was inserted into the FrameBuffer.
+    vp9_fix_pid_offset_ = ForwardDiff<uint16_t, kPicIdLength>(
+        *picture_id, vp9_fix_last_picture_id_);
+    vp9_fix_pid_offset_ += 128;

[stefan-webrtc, 2016/11/14 12:00:26]

Can we name this constant in a good way? Maybe it's already named somewhere?

[philipel, 2016/11/14 15:31:03]

Done.

+
+    fixed_pid = Add<kPicIdLength>(*picture_id, vp9_fix_pid_offset_);
+    vp9_fix_last_picture_id_ = fixed_pid;
+    vp9_fix_jump_timestamp_ = frame.timestamp;
+    gof_info_.clear();
+
+    vp9_fix_tl0_pic_idx_offset_ =
+        ForwardDiff<uint8_t>(*tl0_pic_idx, vp9_fix_last_tl0_pic_idx_);
+    vp9_fix_tl0_pic_idx_offset_ += kMaxGofSaved;
+    fixed_tl0 = Add<256>(*tl0_pic_idx, vp9_fix_tl0_pic_idx_offset_);
+    vp9_fix_last_tl0_pic_idx_ = fixed_tl0;
+  }
+
+  // Update |vp9_fix_last_picture_id_| with the most recent picture id.
+  if (AheadOf<uint16_t, kPicIdLength>(fixed_pid, vp9_fix_last_picture_id_))
+    vp9_fix_last_picture_id_ = fixed_pid;
+
+  *picture_id = fixed_pid;
+  *tl0_pic_idx = fixed_tl0;
+
+  return false;
+}
+
 }  // namespace video_coding
 }  // namespace webrtc