Request keyframes more frequently on stream start/decoding error.

webrtc/video/video_receive_stream.cc

Index: webrtc/video/video_receive_stream.cc
diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc
index e8a9da22455c7a79d9b2948071347103987496f7..52da09a82df4444d7401a28703a7161980ed34f0 100644
--- a/webrtc/video/video_receive_stream.cc
+++ b/webrtc/video/video_receive_stream.cc
@@ -491,9 +491,12 @@ void VideoReceiveStream::DecodeThreadFunction(void* ptr) {
 bool VideoReceiveStream::Decode() {
   TRACE_EVENT0("webrtc", "VideoReceiveStream::Decode");
   static const int kMaxWaitForFrameMs = 3000;
+  static const int kMaxWaitForKeyFrameMs = 200;

[terelius, 2017/08/07 12:00:06]

Isn't 200 ms a bit short? It will take at least 1 RTT to get a new keyframe
after a decoding error, right? What happens if no frame is available within the
time limit?

[philipel, 2017/08/07 12:09:07]

I agree that waiting an RTT between requests is better. I was hopping this CL
(https://codereview.chromium.org/2980413002) would land sooner, but maybe I
should just fix it and land it myself.

[noahric, 2017/08/07 15:12:29]

I don't think you want to wait a full RTT when RTT is high (when it's low,
maybe, but you probably also don't want to generate keyframes faster than
something on the order of 100ms). Yes, you may end up requesting multiple, but
the cost of that is more reasonable than requests being lost and having to wait
multiples of RTT to restart the sequence (e.g. a 500ms RTT means at least 1
second of stopped video if the first request is lost, whereas requesting it
every 200ms means you may get two keyframes but you'll hopefully get a keyframe
within 500, 700, or 900ms).

I picked 200 kinda randomly, but it seemed reasonable, and I thought the old
jitter buffer (? or something) had keyframe requests throttled at around 200ms.

[philipel, 2017/08/07 16:27:49]

The old jitter buffer requested a new KF every 500 ms in the case of a decoding
error.

I know it's not as bad for video, but for screensharing sending an extra
keyframe can be quite costly.

I still think waiting an RTT is preferable, and I will most likely use that
instead of 200 ms when the CL mentioned above has landed.

[noahric, 2017/08/07 16:48:44]

If a decoder requests keyframes on every incoming frame because it can't decode,
is there throttling on that? Or does that trigger the needs_keyframe state now
and so this code will throttle it?
Because it's higher resolution? Or has more complexity at a given resolution
(I'm curious if that's generally true)?

Two things to consider:
1) If that's the case, you could change screencasting specifically to throttle
on the receive side, so it'd only generate a keyframe every ~600-1000ms or so.
So you'd still end up with potentially extra keyframes, but not that many extra,
and you'd still cut down the worst-case delay.
2) Is that cost still worse than not rendering anything in lossy scenarios with
high rtt?

+
+  int wait_ms = keyframe_required_ ? kMaxWaitForKeyFrameMs : kMaxWaitForFrameMs;
   std::unique_ptr<video_coding::FrameObject> frame;
   video_coding::FrameBuffer::ReturnReason res =
-      frame_buffer_->NextFrame(kMaxWaitForFrameMs, &frame);
+      frame_buffer_->NextFrame(wait_ms, &frame, keyframe_required_);
 
   if (res == video_coding::FrameBuffer::ReturnReason::kStopped) {
     video_receiver_.DecodingStopped();
@@ -502,8 +505,12 @@ 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;

[stefan-webrtc, 2017/08/07 11:54:38]

Is this the behavior we want?
https://bugs.chromium.org/p/webrtc/issues/detail?id=8026 suggests that this
isn't enough, and maybe it makes more sense to keep requesting until we actually
get a key frame?

[philipel, 2017/08/07 11:59:34]

I think this CL will solve that problem as well, if we fail to decode we now
require the next frame to be a keyframe (the else case of this if statement).

[stefan-webrtc, 2017/08/07 12:24:53]

Right, I missed that. Do we have test coverage for that?

[philipel, 2017/08/07 13:30:11]

We don't have any tests for VideoReceiveStream except for one testing H264 out
of band parameters. It looks like quite a lot of work to create tests for this
:P

[stefan-webrtc, 2017/08/09 07:02:35]

Ok, perhaps we can write an end-to-end test using a fake decoder which we make
fail and verify that we get multiple key frame requests?

I do think it would make sense to write receive-stream tests though, perhaps
using recorded rtp dumps. For instance we could use the one provided in
https://bugs.chromium.org/p/webrtc/issues/detail?id=8026?

It may be worth doing separately though. Feel free to file a bug. 

       rtp_video_stream_receiver_.FrameDecoded(frame->picture_id);
+    } else {
+      keyframe_required_ = true;

[terelius, 2017/08/07 12:00:05]

Where do we actually send the keyframe request?

[philipel, 2017/08/07 12:09:07]

What happens when we require a keyframe is that we first ask the |frame_buffer_|
for one (lline 499), and if we don't get a keyframe we execute the else block on
line 514, and the keyframe is requested on line 536.

[terelius, 2017/08/07 12:48:56]

Acknowledged.

+    }
   } else {
     RTC_DCHECK_EQ(res, video_coding::FrameBuffer::ReturnReason::kTimeout);
     int64_t now_ms = clock_->TimeInMilliseconds();
@@ -524,7 +531,7 @@ bool VideoReceiveStream::Decode() {
         now_ms - *last_keyframe_packet_ms < kMaxWaitForFrameMs;
 
     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();
     }