From 815e00c10215cce60e66486d9d78826ce7dca622 Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Tue, 12 Nov 2019 15:20:21 +0000 Subject: [PATCH] Revert "Reset RtpFrameReferenceFinder on long pause" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 7a4db6eb0ef5a998019f03428072f0cc6afae866. Reason for revert: Caused regression on perf tests. Original change's description: > Reset RtpFrameReferenceFinder on long pause > > Bug: webrtc:11074 > Change-Id: I4c9a8761e9039d32885ccf9ac0eebdffdf67f48d > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/159240 > Commit-Queue: Ilya Nikolaevskiy > Reviewed-by: Erik Språng > Cr-Commit-Position: refs/heads/master@{#29747} TBR=ilnik@webrtc.org,sprang@webrtc.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:11074 Change-Id: Ic40779087bf8e6bd94f02d38161f6abb9ca395f1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/159690 Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#29775} --- video/rtp_video_stream_receiver.cc | 37 ++++++++---------------------- video/rtp_video_stream_receiver.h | 3 +-- video/video_receive_stream.cc | 9 ++++---- 3 files changed, 14 insertions(+), 35 deletions(-) diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index ea33eb5449..5902886da4 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -40,7 +40,6 @@ #include "rtc_base/logging.h" #include "rtc_base/strings/string_builder.h" #include "rtc_base/system/fallthrough.h" -#include "rtc_base/time_utils.h" #include "system_wrappers/include/field_trial.h" #include "system_wrappers/include/metrics.h" #include "video/receive_statistics_proxy.h" @@ -53,10 +52,6 @@ namespace { constexpr int kPacketBufferStartSize = 512; constexpr int kPacketBufferMaxSize = 2048; -// Maximum time between frames before resetting reference_finder to avoid RTP -// fields wraparounds to affect FrameBuffer. -constexpr TimeDelta kInactiveStreamThreshold = TimeDelta::Seconds<5>(); - int PacketBufferMaxSize() { // The group here must be a positive power of 2, in which case that is used as // size. All other values shall result in the default value being used. @@ -219,9 +214,7 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( rtcp_feedback_buffer_(this, nack_sender, this), packet_buffer_(clock_, kPacketBufferStartSize, PacketBufferMaxSize()), has_received_frame_(false), - frames_decryptable_(false), - last_completed_picture_id_(0), - last_assembled_frame_time_(Timestamp::MinusInfinity()) { + frames_decryptable_(false) { constexpr bool remb_candidate = true; if (packet_router_) packet_router_->AddReceiveRtpModule(rtp_rtcp_.get(), remb_candidate); @@ -604,15 +597,6 @@ void RtpVideoStreamReceiver::OnAssembledFrame( RTC_DCHECK_RUN_ON(&network_tc_); RTC_DCHECK(frame); - bool recreate_reference_finder_requested = false; - - Timestamp now = clock_->CurrentTime(); - if (last_assembled_frame_time_.IsFinite() && - now - last_assembled_frame_time_ > kInactiveStreamThreshold) { - recreate_reference_finder_requested = true; - } - last_assembled_frame_time_ = now; - absl::optional descriptor = frame->GetGenericFrameDescriptor(); @@ -645,7 +629,14 @@ void RtpVideoStreamReceiver::OnAssembledFrame( if (frame->codec_type() != current_codec_) { if (frame_is_newer) { - recreate_reference_finder_requested = true; + // When we reset the |reference_finder_| we don't want new picture ids + // to overlap with old picture ids. To ensure that doesn't happen we + // start from the |last_completed_picture_id_| and add an offset in case + // of reordering. + reference_finder_ = + std::make_unique( + this, last_completed_picture_id_ + + std::numeric_limits::max()); current_codec_ = frame->codec_type(); } else { // Old frame from before the codec switch, discard it. @@ -661,16 +652,6 @@ void RtpVideoStreamReceiver::OnAssembledFrame( last_assembled_frame_rtp_timestamp_ = frame->Timestamp(); } - if (recreate_reference_finder_requested) { - // When we reset the |reference_finder_| we don't want new picture ids - // to overlap with old picture ids. To ensure that doesn't happen we - // start from the |last_completed_picture_id_| and add an offset in case - // of reordering. - reference_finder_ = std::make_unique( - this, - last_completed_picture_id_ + std::numeric_limits::max()); - } - if (buffered_frame_decryptor_ == nullptr) { reference_finder_->ManageFrame(std::move(frame)); } else { diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h index b7f38f84be..7021c3c7dc 100644 --- a/video/rtp_video_stream_receiver.h +++ b/video/rtp_video_stream_receiver.h @@ -319,8 +319,7 @@ class RtpVideoStreamReceiver : public LossNotificationSender, std::atomic frames_decryptable_; absl::optional last_color_space_; - int64_t last_completed_picture_id_; - Timestamp last_assembled_frame_time_ RTC_GUARDED_BY(network_tc_); + int64_t last_completed_picture_id_ = 0; }; } // namespace webrtc diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index aef1c518d7..a60bb07911 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -159,9 +159,10 @@ class EncodedFrameForMediaTransport : public video_coding::EncodedFrame { int64_t RenderTime() const override { return 0; } }; +// TODO(https://bugs.webrtc.org/9974): Consider removing this workaround. // Maximum time between frames before resetting the FrameBuffer to avoid RTP -// timestamps and picture IDs wraparounds to affect FrameBuffer. -constexpr int kInactiveStreamThresholdMs = 5000; // 5 seconds. +// timestamps wraparound to affect FrameBuffer. +constexpr int kInactiveStreamThresholdMs = 600000; // 10 minutes. } // namespace @@ -545,9 +546,7 @@ void VideoReceiveStream::RequestKeyFrame() { void VideoReceiveStream::OnCompleteFrame( std::unique_ptr frame) { RTC_DCHECK_RUN_ON(&network_sequence_checker_); - - // Resetting of stream state if there was long enough pause in the stream. - // This is done to avoid undetected wraparounds in RTP fields. + // TODO(https://bugs.webrtc.org/9974): Consider removing this workaround. int64_t time_now_ms = rtc::TimeMillis(); if (last_complete_frame_time_ms_ > 0 && time_now_ms - last_complete_frame_time_ms_ > kInactiveStreamThresholdMs) {