From 342c5110078c7dccbf1bd2bbdbc97616aef7c5d4 Mon Sep 17 00:00:00 2001 From: philipel Date: Mon, 28 Mar 2022 14:30:06 +0200 Subject: [PATCH] Save unwrapped `tl0_pic_idx` for inserted VP8 frames. As stashed frames are retried their `tl0_pic_idx` are again unwrapped which can lead to the `tl0_unwrapper_` to unwrap the `tl0_pic_idx` of newer frames backwards. Instead unwrap the `tl0_pid_idx` only once and save it with the frame if necessary. Related VP9 CL: https://webrtc-review.googlesource.com/c/src/+/253844 Bug: none Change-Id: I8265dc5f36ee257db92d79cec719f56b165d3855 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/256966 Reviewed-by: Danil Chapovalov Commit-Queue: Philip Eliasson Cr-Commit-Position: refs/heads/main@{#36356} --- modules/video_coding/rtp_vp8_ref_finder.cc | 38 ++++++++++--------- modules/video_coding/rtp_vp8_ref_finder.h | 11 +++++- .../rtp_vp8_ref_finder_unittest.cc | 10 +++++ 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/modules/video_coding/rtp_vp8_ref_finder.cc b/modules/video_coding/rtp_vp8_ref_finder.cc index 64f1bae300..85fe18a263 100644 --- a/modules/video_coding/rtp_vp8_ref_finder.cc +++ b/modules/video_coding/rtp_vp8_ref_finder.cc @@ -18,14 +18,20 @@ namespace webrtc { RtpFrameReferenceFinder::ReturnVector RtpVp8RefFinder::ManageFrame( std::unique_ptr frame) { - FrameDecision decision = ManageFrameInternal(frame.get()); + const RTPVideoHeaderVP8& codec_header = absl::get( + frame->GetRtpVideoHeader().video_type_header); + int64_t unwrapped_tl0 = tl0_unwrapper_.Unwrap(codec_header.tl0PicIdx & 0xFF); + FrameDecision decision = + ManageFrameInternal(frame.get(), codec_header, unwrapped_tl0); RtpFrameReferenceFinder::ReturnVector res; switch (decision) { case kStash: - if (stashed_frames_.size() > kMaxStashedFrames) + if (stashed_frames_.size() > kMaxStashedFrames) { stashed_frames_.pop_back(); - stashed_frames_.push_front(std::move(frame)); + } + stashed_frames_.push_front( + {.unwrapped_tl0 = unwrapped_tl0, .frame = std::move(frame)}); return res; case kHandOff: res.push_back(std::move(frame)); @@ -39,11 +45,9 @@ RtpFrameReferenceFinder::ReturnVector RtpVp8RefFinder::ManageFrame( } RtpVp8RefFinder::FrameDecision RtpVp8RefFinder::ManageFrameInternal( - RtpFrameObject* frame) { - const RTPVideoHeader& video_header = frame->GetRtpVideoHeader(); - const RTPVideoHeaderVP8& codec_header = - absl::get(video_header.video_type_header); - + RtpFrameObject* frame, + const RTPVideoHeaderVP8& codec_header, + int64_t unwrapped_tl0) { // Protect against corrupted packets with arbitrary large temporal idx. if (codec_header.temporalIdx >= kMaxTemporalLayers) return kDrop; @@ -73,8 +77,6 @@ RtpVp8RefFinder::FrameDecision RtpVp8RefFinder::ManageFrameInternal( } while (last_picture_id_ != frame->Id()); } - int64_t unwrapped_tl0 = tl0_unwrapper_.Unwrap(codec_header.tl0PicIdx & 0xFF); - // Clean up info for base layers that are too old. int64_t old_tl0_pic_idx = unwrapped_tl0 - kMaxLayerInfo; auto clean_layer_info_to = layer_info_.lower_bound(old_tl0_pic_idx); @@ -207,20 +209,22 @@ void RtpVp8RefFinder::RetryStashedFrames( bool complete_frame = false; do { complete_frame = false; - for (auto frame_it = stashed_frames_.begin(); - frame_it != stashed_frames_.end();) { - FrameDecision decision = ManageFrameInternal(frame_it->get()); + for (auto it = stashed_frames_.begin(); it != stashed_frames_.end();) { + const RTPVideoHeaderVP8& codec_header = absl::get( + it->frame->GetRtpVideoHeader().video_type_header); + FrameDecision decision = + ManageFrameInternal(it->frame.get(), codec_header, it->unwrapped_tl0); switch (decision) { case kStash: - ++frame_it; + ++it; break; case kHandOff: complete_frame = true; - res.push_back(std::move(*frame_it)); + res.push_back(std::move(it->frame)); [[fallthrough]]; case kDrop: - frame_it = stashed_frames_.erase(frame_it); + it = stashed_frames_.erase(it); } } } while (complete_frame); @@ -235,7 +239,7 @@ void RtpVp8RefFinder::UnwrapPictureIds(RtpFrameObject* frame) { void RtpVp8RefFinder::ClearTo(uint16_t seq_num) { auto it = stashed_frames_.begin(); while (it != stashed_frames_.end()) { - if (AheadOf(seq_num, (*it)->first_seq_num())) { + if (AheadOf(seq_num, it->frame->first_seq_num())) { it = stashed_frames_.erase(it); } else { ++it; diff --git a/modules/video_coding/rtp_vp8_ref_finder.h b/modules/video_coding/rtp_vp8_ref_finder.h index 0a6cd7e10d..1ae45cdba3 100644 --- a/modules/video_coding/rtp_vp8_ref_finder.h +++ b/modules/video_coding/rtp_vp8_ref_finder.h @@ -38,9 +38,16 @@ class RtpVp8RefFinder { static constexpr int kMaxStashedFrames = 100; static constexpr int kMaxTemporalLayers = 5; + struct UnwrappedTl0Frame { + int64_t unwrapped_tl0; + std::unique_ptr frame; + }; + enum FrameDecision { kStash, kHandOff, kDrop }; - FrameDecision ManageFrameInternal(RtpFrameObject* frame); + FrameDecision ManageFrameInternal(RtpFrameObject* frame, + const RTPVideoHeaderVP8& codec_header, + int64_t unwrapped_tl0); void RetryStashedFrames(RtpFrameReferenceFinder::ReturnVector& res); void UpdateLayerInfoVp8(RtpFrameObject* frame, int64_t unwrapped_tl0, @@ -58,7 +65,7 @@ class RtpVp8RefFinder { // Frames that have been fully received but didn't have all the information // needed to determine their references. - std::deque> stashed_frames_; + std::deque stashed_frames_; // Holds the information about the last completed frame for a given temporal // layer given an unwrapped Tl0 picture index. diff --git a/modules/video_coding/rtp_vp8_ref_finder_unittest.cc b/modules/video_coding/rtp_vp8_ref_finder_unittest.cc index a77149a89b..7dc6cd5521 100644 --- a/modules/video_coding/rtp_vp8_ref_finder_unittest.cc +++ b/modules/video_coding/rtp_vp8_ref_finder_unittest.cc @@ -357,4 +357,14 @@ TEST_F(RtpVp8RefFinderTest, Vp8DetectMissingFrame_0212) { EXPECT_THAT(frames_, HasFrameWithIdAndRefs(8, {5, 6, 7})); } +TEST_F(RtpVp8RefFinderTest, StashedFramesDoNotWrapTl0Backwards) { + Insert(Frame().Pid(0).Tid(0).Tl0(0)); + EXPECT_THAT(frames_, SizeIs(0)); + + Insert(Frame().Pid(128).Tid(0).Tl0(128).AsKeyFrame()); + EXPECT_THAT(frames_, SizeIs(1)); + Insert(Frame().Pid(129).Tid(0).Tl0(129)); + EXPECT_THAT(frames_, SizeIs(2)); +} + } // namespace webrtc