From 628ac5964e32e66083a6ab14dceac6cb2cabe345 Mon Sep 17 00:00:00 2001 From: philipel Date: Fri, 11 Aug 2017 03:41:44 -0700 Subject: [PATCH] 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} --- 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, 51 insertions(+), 31 deletions(-) diff --git a/webrtc/modules/video_coding/frame_buffer2.cc b/webrtc/modules/video_coding/frame_buffer2.cc index 2540ae5941..016a24e807 100644 --- a/webrtc/modules/video_coding/frame_buffer2.cc +++ b/webrtc/modules/video_coding/frame_buffer2.cc @@ -59,7 +59,8 @@ FrameBuffer::~FrameBuffer() {} FrameBuffer::ReturnReason FrameBuffer::NextFrame( int64_t max_wait_time_ms, - std::unique_ptr* frame_out) { + std::unique_ptr* frame_out, + bool keyframe_required) { TRACE_EVENT0("webrtc", "FrameBuffer::NextFrame"); int64_t latest_return_time_ms = clock_->TimeInMilliseconds() + max_wait_time_ms; @@ -105,6 +106,10 @@ 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)); @@ -279,7 +284,7 @@ int FrameBuffer::InsertFrame(std::unique_ptr frame) { TRACE_EVENT0("webrtc", "FrameBuffer::InsertFrame"); RTC_DCHECK(frame); if (stats_callback_) - stats_callback_->OnCompleteFrame(frame->num_references == 0, frame->size()); + stats_callback_->OnCompleteFrame(frame->is_keyframe(), frame->size()); FrameKey key(frame->picture_id, frame->spatial_layer); rtc::CritScope lock(&crit_); @@ -307,7 +312,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->num_references == 0) { + frame->is_keyframe()) { // 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 4950dac3ff..8de0680ef9 100644 --- a/webrtc/modules/video_coding/frame_buffer2.h +++ b/webrtc/modules/video_coding/frame_buffer2.h @@ -57,7 +57,8 @@ 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); + std::unique_ptr* frame_out, + bool keyframe_required = false); // 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 d85f1cf233..95a879b36d 100644 --- a/webrtc/modules/video_coding/frame_buffer2_unittest.cc +++ b/webrtc/modules/video_coding/frame_buffer2_unittest.cc @@ -167,11 +167,12 @@ class TestFrameBuffer2 : public ::testing::Test { return buffer_.InsertFrame(std::move(frame)); } - void ExtractFrame(int64_t max_wait_time = 0) { + void ExtractFrame(int64_t max_wait_time = 0, bool keyframe_required = false) { crit_.Enter(); if (max_wait_time == 0) { std::unique_ptr frame; - FrameBuffer::ReturnReason res = buffer_.NextFrame(0, &frame); + FrameBuffer::ReturnReason res = + buffer_.NextFrame(0, &frame, keyframe_required); if (res != FrameBuffer::ReturnReason::kStopped) frames_.emplace_back(std::move(frame)); crit_.Leave(); @@ -540,5 +541,18 @@ 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 0751077041..5299ed8152 100644 --- a/webrtc/modules/video_coding/frame_object.h +++ b/webrtc/modules/video_coding/frame_object.h @@ -44,6 +44,8 @@ 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 3d3e155dc3..de6764f9b5 100644 --- a/webrtc/modules/video_coding/video_receiver.cc +++ b/webrtc/modules/video_coding/video_receiver.cc @@ -320,24 +320,7 @@ int32_t VideoReceiver::Decode(const VCMEncodedFrame& frame) { if (decoder == nullptr) { return VCM_NO_CODEC_REGISTERED; } - // 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; + return decoder->Decode(frame, clock_->TimeInMilliseconds()); } // 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 d40bcb336f..18cba03e21 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -497,9 +497,14 @@ 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(kMaxWaitForFrameMs, &frame); + frame_buffer_->NextFrame(wait_ms, &frame); if (res == video_coding::FrameBuffer::ReturnReason::kStopped) { video_receiver_.DecodingStopped(); @@ -508,8 +513,15 @@ bool VideoReceiveStream::Decode() { if (frame) { RTC_DCHECK_EQ(res, video_coding::FrameBuffer::ReturnReason::kFrameFound); - if (video_receiver_.Decode(frame.get()) == VCM_OK) + if (video_receiver_.Decode(frame.get()) == VCM_OK) { + keyframe_required_ = false; 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(); @@ -522,15 +534,14 @@ 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 (within |kMaxWaitForFrameMs|) have been receiving packets - // belonging to a keyframe then we assume a keyframe is being received right - // now. + // If we recently have been receiving packets belonging to a keyframe then + // we assume a keyframe is currently being received. bool receiving_keyframe = last_keyframe_packet_ms && - now_ms - *last_keyframe_packet_ms < kMaxWaitForFrameMs; + now_ms - *last_keyframe_packet_ms < kMaxWaitForKeyFrameMs; if (stream_is_active && !receiving_keyframe) { - LOG(LS_WARNING) << "No decodable frame in " << kMaxWaitForFrameMs + LOG(LS_WARNING) << "No decodable frame in " << wait_ms << " ms, requesting keyframe."; RequestKeyFrame(); } diff --git a/webrtc/video/video_receive_stream.h b/webrtc/video/video_receive_stream.h index 3ae092fe6e..89c324ce0b 100644 --- a/webrtc/video/video_receive_stream.h +++ b/webrtc/video/video_receive_stream.h @@ -148,6 +148,10 @@ 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