Don't update internal state of the FrameBuffer2 when an undecodable f…

…rame is inserted. Bug: chromium:844313 Change-Id: I034bcb47092815695084e37c81150bafbfbc6b9c Reviewed-on: https://webrtc-review.googlesource.com/79944 Commit-Queue: Philip Eliasson <[email protected]> Reviewed-by: Björn Terelius <[email protected]> Cr-Commit-Position: refs/heads/master@{#23577}
drivestudy · Jun 12, 2018 · 798b282 · 798b282
1 parent dadaaee
commit 798b282
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 44 deletions.
diff --git a/modules/video_coding/frame_buffer2.cc b/modules/video_coding/frame_buffer2.cc
@@ -13,6 +13,7 @@
 #include <algorithm>
 #include <cstring>
 #include <queue>
+#include <vector>
 
 #include "modules/video_coding/include/video_coding_defines.h"
 #include "modules/video_coding/jitter_estimator.h"
@@ -486,20 +487,34 @@ bool FrameBuffer::UpdateFrameInfoWithIncomingFrame(const EncodedFrame& frame,
                                                    FrameMap::iterator info) {
   TRACE_EVENT0("webrtc", "FrameBuffer::UpdateFrameInfoWithIncomingFrame");
   const VideoLayerFrameId& id = frame.id;
-  info->second.num_missing_continuous = frame.num_references;
-  info->second.num_missing_decodable = frame.num_references;
 
   RTC_DCHECK(last_decoded_frame_it_ == frames_.end() ||
              last_decoded_frame_it_->first < info->first);
 
-  // Check how many dependencies that have already been fulfilled.
+  // In this function we determine how many missing dependencies this |frame|
+  // has to become continuous/decodable. If a frame that this |frame| depend
+  // on has already been decoded then we can ignore that dependency since it has
+  // already been fulfilled.
+  //
+  // For all other frames we will register a backwards reference to this |frame|
+  // so that |num_missing_continuous| and |num_missing_decodable| can be
+  // decremented as frames become continuous/are decoded.
+  struct Dependency {
+    VideoLayerFrameId id;
+    bool continuous;
+  };
+  std::vector<Dependency> not_yet_fulfilled_dependencies;
+
+  // Find all dependencies that have not yet been fulfilled.
   for (size_t i = 0; i < frame.num_references; ++i) {
     VideoLayerFrameId ref_key(frame.references[i], frame.id.spatial_layer);
     auto ref_info = frames_.find(ref_key);
 
-    // Does |frame| depend on a frame earlier than the last decoded frame?
+    // Does |frame| depend on a frame earlier than the last decoded one?
     if (last_decoded_frame_it_ != frames_.end() &&
         ref_key <= last_decoded_frame_it_->first) {
+      // Was that frame decoded? If not, this |frame| will never become
+      // decodable.
       if (ref_info == frames_.end()) {
         int64_t now_ms = clock_->TimeInMilliseconds();
         if (last_log_non_decoded_ms_ + kLogNonDecodedIntervalMs < now_ms) {
@@ -512,58 +527,52 @@ bool FrameBuffer::UpdateFrameInfoWithIncomingFrame(const EncodedFrame& frame,
         }
         return false;
       }
-
-      --info->second.num_missing_continuous;
-      --info->second.num_missing_decodable;
     } else {
-      if (ref_info == frames_.end())
-        ref_info = frames_.insert(std::make_pair(ref_key, FrameInfo())).first;
-
-      if (ref_info->second.continuous)
-        --info->second.num_missing_continuous;
-
-      // Add backwards reference so |frame| can be updated when new
-      // frames are inserted or decoded.
-      ref_info->second.dependent_frames[ref_info->second.num_dependent_frames] =
-          id;
-      RTC_DCHECK_LT(ref_info->second.num_dependent_frames,
-                    (FrameInfo::kMaxNumDependentFrames - 1));
-      // TODO(philipel): Look into why this could happen and handle
-      // appropriately.
-      if (ref_info->second.num_dependent_frames <
-          (FrameInfo::kMaxNumDependentFrames - 1)) {
-        ++ref_info->second.num_dependent_frames;
-      }
+      bool ref_continuous =
+          ref_info != frames_.end() && ref_info->second.continuous;
+      not_yet_fulfilled_dependencies.push_back({ref_key, ref_continuous});
     }
-    RTC_DCHECK_LE(ref_info->second.num_missing_continuous,
-                  ref_info->second.num_missing_decodable);
   }
 
-  // Check if we have the lower spatial layer frame.
+  // Does |frame| depend on the lower spatial layer?
   if (frame.inter_layer_predicted) {
-    ++info->second.num_missing_continuous;
-    ++info->second.num_missing_decodable;
-
     VideoLayerFrameId ref_key(frame.id.picture_id, frame.id.spatial_layer - 1);
-    // Gets or create the FrameInfo for the referenced frame.
-    auto ref_info = frames_.insert(std::make_pair(ref_key, FrameInfo())).first;
-    if (ref_info->second.continuous)
+    auto ref_info = frames_.find(ref_key);
+
+    bool lower_layer_continuous =
+        ref_info != frames_.end() && ref_info->second.continuous;
+    bool lower_layer_decoded = last_decoded_frame_it_ != frames_.end() &&
+                               last_decoded_frame_it_->first == ref_key;
+
+    if (!lower_layer_continuous || !lower_layer_decoded) {
+      not_yet_fulfilled_dependencies.push_back(
+          {ref_key, lower_layer_continuous});
+    }
+  }
+
+  info->second.num_missing_continuous = not_yet_fulfilled_dependencies.size();
+  info->second.num_missing_decodable = not_yet_fulfilled_dependencies.size();
+
+  for (const Dependency& dep : not_yet_fulfilled_dependencies) {
+    if (dep.continuous)
       --info->second.num_missing_continuous;
 
-    if (ref_info == last_decoded_frame_it_) {
-      --info->second.num_missing_decodable;
+    // At this point we know we want to insert this frame, so here we
+    // intentionally get or create the FrameInfo for this dependency.
+    FrameInfo* dep_info = &frames_[dep.id];
+
+    if (dep_info->num_dependent_frames <
+        (FrameInfo::kMaxNumDependentFrames - 1)) {
+      dep_info->dependent_frames[dep_info->num_dependent_frames] = id;
+      ++dep_info->num_dependent_frames;
     } else {
-      ref_info->second.dependent_frames[ref_info->second.num_dependent_frames] =
-          id;
-      ++ref_info->second.num_dependent_frames;
+      RTC_LOG(LS_WARNING) << "Frame with (picture_id:spatial_id) ("
+                          << dep.id.picture_id << ":"
+                          << static_cast<int>(dep.id.spatial_layer)
+                          << ") is referenced by too many frames.";
     }
-    RTC_DCHECK_LE(ref_info->second.num_missing_continuous,
-                  ref_info->second.num_missing_decodable);
   }
 
-  RTC_DCHECK_LE(info->second.num_missing_continuous,
-                info->second.num_missing_decodable);
-
   return true;
 }
 

diff --git a/modules/video_coding/frame_buffer2_unittest.cc b/modules/video_coding/frame_buffer2_unittest.cc
@@ -592,5 +592,15 @@ TEST_F(TestFrameBuffer2, KeyframeClearsFullBuffer) {
   CheckFrame(1, kMaxBufferSize + 1, 0);
 }
 
+TEST_F(TestFrameBuffer2, DontUpdateOnUndecodableFrame) {
+  InsertFrame(1, 0, 0, false);
+  ExtractFrame(0, true);
+  InsertFrame(3, 0, 0, false, 2, 0);
+  InsertFrame(3, 0, 0, false, 0);
+  InsertFrame(2, 0, 0, false);
+  ExtractFrame(0, true);
+  ExtractFrame(0, true);
+}
+
 }  // namespace video_coding
 }  // namespace webrtc