Request keyframes more frequently on stream start/decoding error.

webrtc/video/video_receive_stream.cc

 /*
  *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 473 matching lines @@
 }
 
 void VideoReceiveStream::DecodeThreadFunction(void* ptr) {
   while (static_cast<VideoReceiveStream*>(ptr)->Decode()) {
   }
 }
 
 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();
     return false;
   }
 
   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();
     rtc::Optional<int64_t> last_packet_ms =
         rtp_video_stream_receiver_.LastReceivedPacketMs();
     rtc::Optional<int64_t> last_keyframe_packet_ms =
         rtp_video_stream_receiver_.LastReceivedKeyframePacketMs();
 
     // To avoid spamming keyframe requests for a stream that is not active we
     // 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.
     bool receiving_keyframe =
         last_keyframe_packet_ms &&
         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();
     }
   }
   return true;
 }
 }  // namespace internal
 }  // namespace webrtc