From 53959fcc2ba580e7c87231708e5b4af7906f6836 Mon Sep 17 00:00:00 2001 From: tkchin Date: Thu, 17 Aug 2017 11:01:46 -0700 Subject: [PATCH] Revert of quest keyframes more frequently on stream start/decoding error. (patchset #2 id:170001 of https://codereview.webrtc.org/2996823002/ ) Reason for revert: Causes iOS H264 calls received in the background to have increased delay before being able to decode stream from sender due to not having a keyframe. Original issue's description: > Reland of quest keyframes more frequently on stream start/decoding error. (patchset #1 id:1 of https://codereview.chromium.org/2994043002/ ) > > Reason for revert: > Create fix CL. > > Original issue's description: > > Revert of Request keyframes more frequently on stream start/decoding error. (patchset #1 id:1 of https://codereview.webrtc.org/2993793002/ ) > > > > Reason for revert: > > Broke downstream test that was waiting for 5 keyframes to be received within 10 seconds. Maybe the issue is that "stats_callback_->OnCompleteFrame(frame->num_references == 0, ..." was changed to "frame->is_keyframe()"? > > > > Original issue's description: > > > Request keyframes more frequently on stream start/decoding error. > > > > > > In this CL: > > > - Added FrameObject::is_keyframe() convinience function. > > > - Moved logic to request keyframes on decoding error from VideoReceived to > > > VideoReceiveStream. > > > - Added keyframe_required as a parameter to FrameBuffer::NextFrame. > > > > > > BUG=webrtc:8074 > > > > > > Review-Url: https://codereview.webrtc.org/2993793002 > > > Cr-Commit-Position: refs/heads/master@{#19280} > > > Committed: https://chromium.googlesource.com/external/webrtc/+/26b48043581735eed6e36b95fae6f5b1bcf8cfb5 > > > > TBR=terelius@webrtc.org,stefan@webrtc.org,noahric@chromium.org,philipel@webrtc.org > > # Skipping CQ checks because original CL landed less than 1 days ago. > > NOPRESUBMIT=true > > NOTREECHECKS=true > > NOTRY=true > > BUG=webrtc:8074 > > > > Review-Url: https://codereview.webrtc.org/2994043002 > > Cr-Commit-Position: refs/heads/master@{#19295} > > Committed: https://chromium.googlesource.com/external/webrtc/+/77a983185f57628cd5955bd2c0a1bf71c30439bb > > TBR=terelius@webrtc.org,stefan@webrtc.org,noahric@chromium.org,deadbeef@webrtc.org > # Skipping CQ checks because original CL landed less than 1 days ago. > BUG=webrtc:8074 > > Review-Url: https://codereview.webrtc.org/2996823002 > Cr-Commit-Position: refs/heads/master@{#19324} > Committed: https://chromium.googlesource.com/external/webrtc/+/628ac5964e32e66083a6ab14dceac6cb2cabe345 TBR=terelius@webrtc.org,stefan@webrtc.org,noahric@chromium.org,deadbeef@webrtc.org,philipel@webrtc.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=webrtc:8074 Review-Url: https://codereview.webrtc.org/2995153002 Cr-Commit-Position: refs/heads/master@{#19392} --- webrtc/modules/video_coding/frame_buffer2.cc | 11 +++----- webrtc/modules/video_coding/frame_buffer2.h | 3 +-- .../video_coding/frame_buffer2_unittest.cc | 18 ++----------- webrtc/modules/video_coding/frame_object.h | 2 -- webrtc/modules/video_coding/video_receiver.cc | 19 +++++++++++++- webrtc/video/video_receive_stream.cc | 25 ++++++------------- webrtc/video/video_receive_stream.h | 4 --- 7 files changed, 31 insertions(+), 51 deletions(-) diff --git a/webrtc/modules/video_coding/frame_buffer2.cc b/webrtc/modules/video_coding/frame_buffer2.cc index 016a24e807..2540ae5941 100644 --- a/webrtc/modules/video_coding/frame_buffer2.cc +++ b/webrtc/modules/video_coding/frame_buffer2.cc @@ -59,8 +59,7 @@ FrameBuffer::~FrameBuffer() {} FrameBuffer::ReturnReason FrameBuffer::NextFrame( int64_t max_wait_time_ms, - std::unique_ptr* frame_out, - bool keyframe_required) { + std::unique_ptr* frame_out) { TRACE_EVENT0("webrtc", "FrameBuffer::NextFrame"); int64_t latest_return_time_ms = clock_->TimeInMilliseconds() + max_wait_time_ms; @@ -106,10 +105,6 @@ FrameBuffer::ReturnReason FrameBuffer::NextFrame( } FrameObject* frame = frame_it->second.frame.get(); - - if (keyframe_required && !frame->is_keyframe()) - continue; - next_frame_it_ = frame_it; if (frame->RenderTime() == -1) frame->SetRenderTime(timing_->RenderTimeMs(frame->timestamp, now_ms)); @@ -284,7 +279,7 @@ int FrameBuffer::InsertFrame(std::unique_ptr frame) { TRACE_EVENT0("webrtc", "FrameBuffer::InsertFrame"); RTC_DCHECK(frame); if (stats_callback_) - stats_callback_->OnCompleteFrame(frame->is_keyframe(), frame->size()); + stats_callback_->OnCompleteFrame(frame->num_references == 0, frame->size()); FrameKey key(frame->picture_id, frame->spatial_layer); rtc::CritScope lock(&crit_); @@ -312,7 +307,7 @@ int FrameBuffer::InsertFrame(std::unique_ptr frame) { if (last_decoded_frame_it_ != frames_.end() && key <= last_decoded_frame_it_->first) { if (AheadOf(frame->timestamp, last_decoded_frame_timestamp_) && - frame->is_keyframe()) { + frame->num_references == 0) { // If this frame has a newer timestamp but an earlier picture id then we // assume there has been a jump in the picture id due to some encoder // reconfiguration or some other reason. Even though this is not according diff --git a/webrtc/modules/video_coding/frame_buffer2.h b/webrtc/modules/video_coding/frame_buffer2.h index 8de0680ef9..4950dac3ff 100644 --- a/webrtc/modules/video_coding/frame_buffer2.h +++ b/webrtc/modules/video_coding/frame_buffer2.h @@ -57,8 +57,7 @@ class FrameBuffer { // kTimeout. // - If the FrameBuffer is stopped then it will return kStopped. ReturnReason NextFrame(int64_t max_wait_time_ms, - std::unique_ptr* frame_out, - bool keyframe_required = false); + std::unique_ptr* frame_out); // Tells the FrameBuffer which protection mode that is in use. Affects // the frame timing. diff --git a/webrtc/modules/video_coding/frame_buffer2_unittest.cc b/webrtc/modules/video_coding/frame_buffer2_unittest.cc index 88f841048c..122dd3d8de 100644 --- a/webrtc/modules/video_coding/frame_buffer2_unittest.cc +++ b/webrtc/modules/video_coding/frame_buffer2_unittest.cc @@ -168,12 +168,11 @@ class TestFrameBuffer2 : public ::testing::Test { return buffer_.InsertFrame(std::move(frame)); } - void ExtractFrame(int64_t max_wait_time = 0, bool keyframe_required = false) { + void ExtractFrame(int64_t max_wait_time = 0) { crit_.Enter(); if (max_wait_time == 0) { std::unique_ptr frame; - FrameBuffer::ReturnReason res = - buffer_.NextFrame(0, &frame, keyframe_required); + FrameBuffer::ReturnReason res = buffer_.NextFrame(0, &frame); if (res != FrameBuffer::ReturnReason::kStopped) frames_.emplace_back(std::move(frame)); crit_.Leave(); @@ -542,18 +541,5 @@ TEST_F(TestFrameBuffer2, InvalidReferences) { EXPECT_EQ(2, InsertFrame(2, 0, 3000, false, 1)); } -TEST_F(TestFrameBuffer2, KeyframeRequired) { - EXPECT_EQ(1, InsertFrame(1, 0, 1000, false)); - EXPECT_EQ(2, InsertFrame(2, 0, 2000, false, 1)); - EXPECT_EQ(3, InsertFrame(3, 0, 3000, false)); - ExtractFrame(); - ExtractFrame(0, true); - ExtractFrame(); - - CheckFrame(0, 1, 0); - CheckFrame(1, 3, 0); - CheckNoFrame(2); -} - } // namespace video_coding } // namespace webrtc diff --git a/webrtc/modules/video_coding/frame_object.h b/webrtc/modules/video_coding/frame_object.h index 5299ed8152..0751077041 100644 --- a/webrtc/modules/video_coding/frame_object.h +++ b/webrtc/modules/video_coding/frame_object.h @@ -44,8 +44,6 @@ class FrameObject : public webrtc::VCMEncodedFrame { size_t size() const { return _length; } - bool is_keyframe() const { return num_references == 0; } - // The tuple (|picture_id|, |spatial_layer|) uniquely identifies a frame // object. For codec types that don't necessarily have picture ids they // have to be constructed from the header data relevant to that codec. diff --git a/webrtc/modules/video_coding/video_receiver.cc b/webrtc/modules/video_coding/video_receiver.cc index de6764f9b5..3d3e155dc3 100644 --- a/webrtc/modules/video_coding/video_receiver.cc +++ b/webrtc/modules/video_coding/video_receiver.cc @@ -320,7 +320,24 @@ int32_t VideoReceiver::Decode(const VCMEncodedFrame& frame) { if (decoder == nullptr) { return VCM_NO_CODEC_REGISTERED; } - return decoder->Decode(frame, clock_->TimeInMilliseconds()); + // Decode a frame + int32_t ret = decoder->Decode(frame, clock_->TimeInMilliseconds()); + + // Check for failed decoding, run frame type request callback if needed. + bool request_key_frame = false; + if (ret < 0) { + request_key_frame = true; + } + + if (!frame.Complete() || frame.MissingFrame()) { + request_key_frame = true; + ret = VCM_OK; + } + if (request_key_frame) { + rtc::CritScope cs(&process_crit_); + _scheduleKeyRequest = true; + } + return ret; } // Register possible receive codecs, can be called multiple times diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index 18cba03e21..d40bcb336f 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -497,14 +497,9 @@ void VideoReceiveStream::DecodeThreadFunction(void* ptr) { bool VideoReceiveStream::Decode() { TRACE_EVENT0("webrtc", "VideoReceiveStream::Decode"); static const int kMaxWaitForFrameMs = 3000; - static const int kMaxWaitForKeyFrameMs = 200; - - int wait_ms = keyframe_required_ ? kMaxWaitForKeyFrameMs : kMaxWaitForFrameMs; std::unique_ptr frame; - // TODO(philipel): Call NextFrame with |keyframe_required| argument when - // downstream project has been fixed. video_coding::FrameBuffer::ReturnReason res = - frame_buffer_->NextFrame(wait_ms, &frame); + frame_buffer_->NextFrame(kMaxWaitForFrameMs, &frame); if (res == video_coding::FrameBuffer::ReturnReason::kStopped) { video_receiver_.DecodingStopped(); @@ -513,15 +508,8 @@ bool VideoReceiveStream::Decode() { if (frame) { RTC_DCHECK_EQ(res, video_coding::FrameBuffer::ReturnReason::kFrameFound); - if (video_receiver_.Decode(frame.get()) == VCM_OK) { - keyframe_required_ = false; + if (video_receiver_.Decode(frame.get()) == VCM_OK) rtp_video_stream_receiver_.FrameDecoded(frame->picture_id); - } else if (keyframe_required_ == false) { - keyframe_required_ = true; - // TODO(philipel): Remove this keyframe request when downstream project - // has been fixed. - RequestKeyFrame(); - } } else { RTC_DCHECK_EQ(res, video_coding::FrameBuffer::ReturnReason::kTimeout); int64_t now_ms = clock_->TimeInMilliseconds(); @@ -534,14 +522,15 @@ bool VideoReceiveStream::Decode() { // check if we have received a packet within the last 5 seconds. bool stream_is_active = last_packet_ms && now_ms - *last_packet_ms < 5000; - // If we recently have been receiving packets belonging to a keyframe then - // we assume a keyframe is currently being received. + // If we recently (within |kMaxWaitForFrameMs|) have been receiving packets + // belonging to a keyframe then we assume a keyframe is being received right + // now. bool receiving_keyframe = last_keyframe_packet_ms && - now_ms - *last_keyframe_packet_ms < kMaxWaitForKeyFrameMs; + now_ms - *last_keyframe_packet_ms < kMaxWaitForFrameMs; if (stream_is_active && !receiving_keyframe) { - LOG(LS_WARNING) << "No decodable frame in " << wait_ms + LOG(LS_WARNING) << "No decodable frame in " << kMaxWaitForFrameMs << " ms, requesting keyframe."; RequestKeyFrame(); } diff --git a/webrtc/video/video_receive_stream.h b/webrtc/video/video_receive_stream.h index 89c324ce0b..3ae092fe6e 100644 --- a/webrtc/video/video_receive_stream.h +++ b/webrtc/video/video_receive_stream.h @@ -148,10 +148,6 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, std::unique_ptr media_receiver_; std::unique_ptr rtx_receiver_; - - // Whenever we are in an undecodable state (stream has just started or due to - // a decoding error) we require a keyframe to restart the stream. - bool keyframe_required_ = true; }; } // namespace internal } // namespace webrtc