Add support for handling reordered SS data on the receive-side for VP9.

webrtc/modules/video_coding/main/source/jitter_buffer.cc

Index: webrtc/modules/video_coding/main/source/jitter_buffer.cc
diff --git a/webrtc/modules/video_coding/main/source/jitter_buffer.cc b/webrtc/modules/video_coding/main/source/jitter_buffer.cc
index 49c2325d80a852971edb1dff629b9c5a997e27e7..c855712c26a2b332beef0f810da331144703ee07 100644
--- a/webrtc/modules/video_coding/main/source/jitter_buffer.cc
+++ b/webrtc/modules/video_coding/main/source/jitter_buffer.cc
@@ -30,6 +30,9 @@
 
 namespace webrtc {
 
+// Interval for updating SS data.
+static const uint32_t kPeriodicUpdateIntervalSec = 60;
+
 // Use this rtt if no value has been reported.
 static const int64_t kDefaultRtt = 200;
 
@@ -113,6 +116,106 @@ void FrameList::Reset(UnorderedFrameList* free_frames) {
   }
 }
 
+bool Vp9SsMap::Insert(const VCMPacket& packet) {
+  if (!packet.codecSpecificHeader.codecHeader.VP9.ss_data_available)
+    return false;
+
+  ss_map_[packet.timestamp] = packet.codecSpecificHeader.codecHeader.VP9.gof;
+  return true;
+}
+
+void Vp9SsMap::Reset() {
+  ss_map_.clear();
+}
+
+bool Vp9SsMap::Find(uint32_t timestamp, uint32_t* ss_timestamp) const {
+  bool found = false;
+  for (const auto& it : ss_map_) {
+    if (it.first == timestamp || IsNewerTimestamp(timestamp, it.first)) {
+      *ss_timestamp = it.first;
+      found = true;
+    }
+  }
+  return found;
+}

[stefan-webrtc, 2015/10/13 13:49:43]

You may be able to implement this with ss_map_.lower_bound(timestamp) in some
way, not sure if it would be worth it though. Probably not since you will only
have one or two SS here, right?

[åsapersson, 2015/10/14 15:13:51]

Should be around one or two (or as many that are received during a minute (old
currently cleaned up each minute)).

[stefan-webrtc, 2015/10/16 06:45:05]

Acknowledged.

+
+void Vp9SsMap::RemoveOld(uint32_t timestamp) {
+  if (!TimeForPeriodicUpdate(timestamp))
+    return;
+
+  uint32_t ss_timestamp;
+  if (!Find(timestamp, &ss_timestamp))

[stefan-webrtc, 2015/10/13 13:49:43]

It would be nice if Find() instead could return an iterator so that you can
simply use that to do an erase.

ss_map_.erase(ss_map_.begin(), it);

[åsapersson, 2015/10/14 15:13:50]

Done.

+    return;
+
+  RemoveOlderThan(ss_timestamp);
+  AdvanceFront(timestamp);
+}
+
+bool Vp9SsMap::TimeForPeriodicUpdate(uint32_t timestamp) const {

[stefan-webrtc, 2015/10/13 13:49:43]

What exactly is a periodic update?

[åsapersson, 2015/10/14 15:13:51]

Renamed this function. Looks for if it is time to do a clean up. Thought it
should not be done too often? This is also advancing the oldest SS data to
handle timestamp wrap in case the SS data are received very seldom (e.g. only
once in beginning, second when IsNewerTimestamp not true).

[stefan-webrtc, 2015/10/16 06:45:05]

Please add this as a comment to the method in the .h file.

[åsapersson, 2015/10/16 10:52:26]

Done.

+  if (ss_map_.empty() || !IsNewerTimestamp(timestamp, ss_map_.begin()->first))
+    return false;
+
+  uint32_t diff_sec = (timestamp - ss_map_.begin()->first) / 90000U;

[stefan-webrtc, 2015/10/13 13:49:43]

Name the constant 90000. We should already have that constant somewhere, right?

[åsapersson, 2015/10/14 15:13:50]

Done.

+  return diff_sec >= kPeriodicUpdateIntervalSec;
+}
+
+void Vp9SsMap::RemoveOlderThan(uint32_t timestamp) {
+  while (!ss_map_.empty() &&
+      IsNewerTimestamp(timestamp, ss_map_.begin()->first)) {
+    ss_map_.erase(ss_map_.begin());
+  }
+}
+
+void Vp9SsMap::AdvanceFront(uint32_t timestamp) {
+  assert(!ss_map_.empty());
+  GofInfoVP9 gof = ss_map_.begin()->second;
+  ss_map_.erase(ss_map_.begin());
+  ss_map_[timestamp] = gof;

[stefan-webrtc, 2015/10/13 13:49:43]

How is this supposed to work? Looks like we are changing the timestamp of the
oldest gof? Why?

[åsapersson, 2015/10/14 15:13:51]

See comment above.

+}
+
+bool Vp9SsMap::UpdatePacket(const VCMPacket* packet)  {

[stefan-webrtc, 2015/10/13 13:49:43]

Can this method be const?

[åsapersson, 2015/10/14 15:13:50]

Done.

+  uint8_t gof_idx = packet->codecSpecificHeader.codecHeader.VP9.gof_idx;
+  if (gof_idx == kNoGofIdx)
+    return false;  // No update needed.
+
+  uint32_t ss_timestamp;
+  if (!Find(packet->timestamp, &ss_timestamp))
+    return false;  // Corresponding SS not yet received.
+
+  auto it = ss_map_.find(ss_timestamp);
+  if (gof_idx >= it->second.num_frames_in_gof)
+    return false;  // Corresponding SS not yet received.
+
+  RTPVideoTypeHeader* hdr = const_cast<RTPVideoTypeHeader*>(

[stefan-webrtc, 2015/10/13 13:49:43]

Preferably no const_casts... Can we do it in some other way? At least do the
const_cast when calling the method.

[åsapersson, 2015/10/14 15:13:50]

Moved const_cast to when calling function. 

+      &packet->codecSpecificHeader.codecHeader);
+  hdr->VP9.temporal_idx = it->second.temporal_idx[gof_idx];
+  hdr->VP9.temporal_up_switch = it->second.temporal_up_switch[gof_idx];
+
+  // TODO(asapersson): Set hdr->VP9.ref_picture_id[i] and add usage.
+  hdr->VP9.num_ref_pics = it->second.num_ref_pics[gof_idx];
+  for (size_t i = 0; i < it->second.num_ref_pics[gof_idx]; ++i) {
+    hdr->VP9.pid_diff[i] = it->second.pid_diff[gof_idx][i];
+  }
+  return true;
+}
+
+void Vp9SsMap::UpdateFrames(FrameList* frames) {
+  for (FrameList::iterator it = frames->begin(); it != frames->end(); ++it) {

[stefan-webrtc, 2015/10/13 13:49:43]

for (auto& it : *frames)

[åsapersson, 2015/10/14 15:13:51]

Done.

+    uint8_t gof_idx = it->second->CodecSpecific()->codecSpecific.VP9.gof_idx;
+    if (gof_idx == kNoGofIdx) {
+      continue;
+    }
+    uint32_t ss_timestamp;
+    if (Find(it->second->TimeStamp(), &ss_timestamp)) {
+      auto ss_iter = ss_map_.find(ss_timestamp);

[stefan-webrtc, 2015/10/13 13:49:42]

Wouldn't it be better to have Find() return this iterator?

[åsapersson, 2015/10/14 15:13:51]

Done.

+      if (gof_idx >= ss_iter->second.num_frames_in_gof) {
+        continue;

[stefan-webrtc, 2015/10/13 13:49:43]

Does this mean we haven't received the right SS yet? Or should we log a warning?

[åsapersson, 2015/10/14 15:13:51]

The right not yet received, added a comment.

+      }
+      it->second->SetGofInfo(ss_iter->second, gof_idx);
+    }
+  }
+}
+
 VCMJitterBuffer::VCMJitterBuffer(Clock* clock,
                                  rtc::scoped_ptr<EventWrapper> event)
     : clock_(clock),
@@ -125,8 +228,6 @@ VCMJitterBuffer::VCMJitterBuffer(Clock* clock,
       incomplete_frames_(),
       last_decoded_state_(),
       first_packet_since_reset_(true),
-      last_gof_timestamp_(0),
-      last_gof_valid_(false),
       stats_callback_(NULL),
       incoming_frame_rate_(0),
       incoming_frame_count_(0),
@@ -222,7 +323,7 @@ void VCMJitterBuffer::Start() {
   first_packet_since_reset_ = true;
   rtt_ms_ = kDefaultRtt;
   last_decoded_state_.Reset();
-  last_gof_valid_ = false;
+  vp9_ss_map_.Reset();
 }
 
 void VCMJitterBuffer::Stop() {
@@ -230,7 +331,7 @@ void VCMJitterBuffer::Stop() {
   UpdateHistograms();
   running_ = false;
   last_decoded_state_.Reset();
-  last_gof_valid_ = false;
+  vp9_ss_map_.Reset();
 
   // Make sure all frames are free and reset.
   for (FrameList::iterator it = decodable_frames_.begin();
@@ -262,7 +363,7 @@ void VCMJitterBuffer::Flush() {
   decodable_frames_.Reset(&free_frames_);
   incomplete_frames_.Reset(&free_frames_);
   last_decoded_state_.Reset();  // TODO(mikhal): sync reset.
-  last_gof_valid_ = false;
+  vp9_ss_map_.Reset();
   num_consecutive_old_packets_ = 0;
   // Also reset the jitter and delay estimates
   jitter_estimate_.Reset();
@@ -593,35 +694,17 @@ VCMFrameBufferEnum VCMJitterBuffer::InsertPacket(const VCMPacket& packet,
   }
 
   if (packet.codec == kVideoCodecVP9) {
-    // TODO(asapersson): Move this code to appropriate place.
-    // TODO(asapersson): Handle out of order GOF.
     if (packet.codecSpecificHeader.codecHeader.VP9.flexible_mode) {
       // TODO(asapersson): Add support for flexible mode.
       return kGeneralError;
     }
-    if (packet.codecSpecificHeader.codecHeader.VP9.ss_data_available) {
-      if (!last_gof_valid_ ||
-          IsNewerTimestamp(packet.timestamp, last_gof_timestamp_)) {
-        last_gof_.CopyGofInfoVP9(
-            packet.codecSpecificHeader.codecHeader.VP9.gof);
-        last_gof_timestamp_ = packet.timestamp;
-        last_gof_valid_ = true;
-      }
-    }
-    if (last_gof_valid_ &&
-        !packet.codecSpecificHeader.codecHeader.VP9.flexible_mode) {
-      uint8_t gof_idx = packet.codecSpecificHeader.codecHeader.VP9.gof_idx;
-      if (gof_idx != kNoGofIdx) {
-        if (gof_idx >= last_gof_.num_frames_in_gof) {
-          LOG(LS_WARNING) << "Incorrect gof_idx: " << gof_idx;
-          return kGeneralError;
-        }
-        RTPVideoTypeHeader* hdr = const_cast<RTPVideoTypeHeader*>(
-            &packet.codecSpecificHeader.codecHeader);
-        hdr->VP9.temporal_idx = last_gof_.temporal_idx[gof_idx];
-        hdr->VP9.temporal_up_switch = last_gof_.temporal_up_switch[gof_idx];
-      }
+    if (vp9_ss_map_.Insert(packet)) {
+      vp9_ss_map_.UpdateFrames(&incomplete_frames_);
     }
+    vp9_ss_map_.UpdatePacket(&packet);
+
+    if (!last_decoded_state_.in_initial_state())
+      vp9_ss_map_.RemoveOld(last_decoded_state_.time_stamp());

[stefan-webrtc, 2015/10/13 13:49:43]

Can you explain why this is needed now?

[åsapersson, 2015/10/14 15:13:50]

To not make the map possibly grow too large.

   }
 
   num_consecutive_old_packets_ = 0;