From 798b28279e7dea21a71adfe4be011ec7e7e72d27 Mon Sep 17 00:00:00 2001 From: philipel Date: Mon, 11 Jun 2018 13:10:14 +0200 Subject: [PATCH] Don't update internal state of the FrameBuffer2 when an undecodable frame is inserted. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: chromium:844313 Change-Id: I034bcb47092815695084e37c81150bafbfbc6b9c Reviewed-on: https://webrtc-review.googlesource.com/79944 Commit-Queue: Philip Eliasson Reviewed-by: Björn Terelius Cr-Commit-Position: refs/heads/master@{#23577} --- modules/video_coding/frame_buffer2.cc | 99 ++++++++++--------- .../video_coding/frame_buffer2_unittest.cc | 10 ++ 2 files changed, 64 insertions(+), 45 deletions(-) diff --git a/modules/video_coding/frame_buffer2.cc b/modules/video_coding/frame_buffer2.cc index 2a437d78b2..37feae16ef 100644 --- a/modules/video_coding/frame_buffer2.cc +++ b/modules/video_coding/frame_buffer2.cc @@ -13,6 +13,7 @@ #include #include #include +#include #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 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,57 +527,51 @@ 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) - --info->second.num_missing_continuous; + auto ref_info = frames_.find(ref_key); - if (ref_info == last_decoded_frame_it_) { - --info->second.num_missing_decodable; - } else { - ref_info->second.dependent_frames[ref_info->second.num_dependent_frames] = - id; - ++ref_info->second.num_dependent_frames; + 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}); } - 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); + 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; + + // 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 { + RTC_LOG(LS_WARNING) << "Frame with (picture_id:spatial_id) (" + << dep.id.picture_id << ":" + << static_cast(dep.id.spatial_layer) + << ") is referenced by too many frames."; + } + } return true; } diff --git a/modules/video_coding/frame_buffer2_unittest.cc b/modules/video_coding/frame_buffer2_unittest.cc index 304910a9a5..1378be5a84 100644 --- 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