From 112adf9ca9e2ea3bc251345fbac46e1d22ed0a57 Mon Sep 17 00:00:00 2001 From: philipel Date: Thu, 15 Jun 2017 09:06:21 -0700 Subject: [PATCH] Validate references of frames inserted into FrameBuffer2. BUG=chromium:730603 Review-Url: https://codereview.webrtc.org/2937243002 Cr-Commit-Position: refs/heads/master@{#18614} --- webrtc/modules/video_coding/frame_buffer2.cc | 42 ++++++++++++++----- webrtc/modules/video_coding/frame_buffer2.h | 5 +++ .../video_coding/frame_buffer2_unittest.cc | 8 ++++ webrtc/modules/video_coding/frame_object.h | 2 + 4 files changed, 46 insertions(+), 11 deletions(-) diff --git a/webrtc/modules/video_coding/frame_buffer2.cc b/webrtc/modules/video_coding/frame_buffer2.cc index de96a52b1b..1114e7cc0a 100644 --- a/webrtc/modules/video_coding/frame_buffer2.cc +++ b/webrtc/modules/video_coding/frame_buffer2.cc @@ -238,6 +238,22 @@ void FrameBuffer::Stop() { new_continuous_frame_event_.Set(); } +bool FrameBuffer::ValidReferences(const FrameObject& frame) const { + for (size_t i = 0; i < frame.num_references; ++i) { + if (AheadOrAt(frame.references[i], frame.picture_id)) + return false; + for (size_t j = i + 1; j < frame.num_references; ++j) { + if (frame.references[i] == frame.references[j]) + return false; + } + } + + if (frame.inter_layer_predicted && frame.spatial_layer == 0) + return false; + + return true; +} + void FrameBuffer::UpdatePlayoutDelays(const FrameObject& frame) { TRACE_EVENT0("webrtc", "FrameBuffer::UpdatePlayoutDelays"); PlayoutDelay playout_delay = frame.EncodedImage().playout_delay_; @@ -262,6 +278,13 @@ int FrameBuffer::InsertFrame(std::unique_ptr frame) { ? -1 : last_continuous_frame_it_->first.picture_id; + if (!ValidReferences(*frame)) { + LOG(LS_WARNING) << "Frame with (picture_id:spatial_id) (" << key.picture_id + << ":" << static_cast(key.spatial_layer) + << ") has invalid frame references, dropping frame."; + return last_continuous_picture_id; + } + if (num_frames_buffered_ >= kMaxFramesBuffered) { LOG(LS_WARNING) << "Frame with (picture_id:spatial_id) (" << key.picture_id << ":" << static_cast(key.spatial_layer) @@ -270,13 +293,6 @@ int FrameBuffer::InsertFrame(std::unique_ptr frame) { return last_continuous_picture_id; } - if (frame->inter_layer_predicted && frame->spatial_layer == 0) { - LOG(LS_WARNING) << "Frame with (picture_id:spatial_id) (" << key.picture_id - << ":" << static_cast(key.spatial_layer) - << ") is marked as inter layer predicted, dropping frame."; - return last_continuous_picture_id; - } - if (last_decoded_frame_it_ != frames_.end() && key <= last_decoded_frame_it_->first) { if (AheadOf(frame->timestamp, last_decoded_frame_timestamp_) && @@ -363,11 +379,15 @@ void FrameBuffer::PropagateContinuity(FrameMap::iterator start) { // any unfulfilled dependencies then that frame is continuous as well. for (size_t d = 0; d < frame->second.num_dependent_frames; ++d) { auto frame_ref = frames_.find(frame->second.dependent_frames[d]); - --frame_ref->second.num_missing_continuous; + RTC_DCHECK(frame_ref != frames_.end()); - if (frame_ref->second.num_missing_continuous == 0) { - frame_ref->second.continuous = true; - continuous_frames.push(frame_ref); + // TODO(philipel): Look into why we've seen this happen. + if (frame_ref != frames_.end()) { + --frame_ref->second.num_missing_continuous; + if (frame_ref->second.num_missing_continuous == 0) { + frame_ref->second.continuous = true; + continuous_frames.push(frame_ref); + } } } } diff --git a/webrtc/modules/video_coding/frame_buffer2.h b/webrtc/modules/video_coding/frame_buffer2.h index 6ce3b4be57..ffeb2aa7ca 100644 --- a/webrtc/modules/video_coding/frame_buffer2.h +++ b/webrtc/modules/video_coding/frame_buffer2.h @@ -97,6 +97,8 @@ class FrameBuffer { // Which other frames that have direct unfulfilled dependencies // on this frame. + // TODO(philipel): Add simple modify/access functions to prevent adding too + // many |dependent_frames|. FrameKey dependent_frames[kMaxNumDependentFrames]; size_t num_dependent_frames = 0; @@ -120,6 +122,9 @@ class FrameBuffer { using FrameMap = std::map; + // Check that the references of |frame| are valid. + bool ValidReferences(const FrameObject& frame) const; + // Updates the minimal and maximal playout delays // depending on the frame. void UpdatePlayoutDelays(const FrameObject& frame) diff --git a/webrtc/modules/video_coding/frame_buffer2_unittest.cc b/webrtc/modules/video_coding/frame_buffer2_unittest.cc index 488b785335..58b3f7a42b 100644 --- a/webrtc/modules/video_coding/frame_buffer2_unittest.cc +++ b/webrtc/modules/video_coding/frame_buffer2_unittest.cc @@ -531,5 +531,13 @@ TEST_F(TestFrameBuffer2, DuplicateFrames) { EXPECT_EQ(22256, InsertFrame(22256, 0, 1, false)); } +// TODO(philipel): implement more unittests related to invalid references. +TEST_F(TestFrameBuffer2, InvalidReferences) { + EXPECT_EQ(-1, InsertFrame(0, 0, 1000, false, 2)); + EXPECT_EQ(1, InsertFrame(1, 0, 2000, false)); + ExtractFrame(); + EXPECT_EQ(2, InsertFrame(2, 0, 3000, false, 1)); +} + } // namespace video_coding } // namespace webrtc diff --git a/webrtc/modules/video_coding/frame_object.h b/webrtc/modules/video_coding/frame_object.h index 413cc33910..fa0a2c1d3d 100644 --- a/webrtc/modules/video_coding/frame_object.h +++ b/webrtc/modules/video_coding/frame_object.h @@ -51,6 +51,8 @@ class FrameObject : public webrtc::VCMEncodedFrame { uint8_t spatial_layer; uint32_t timestamp; + // TODO(philipel): Add simple modify/access functions to prevent adding too + // many |references|. size_t num_references; uint16_t references[kMaxFrameReferences]; bool inter_layer_predicted;