Request keyframe if the first received frame is not a keyframe.

webrtc/video/rtp_stream_receiver.cc

Index: webrtc/video/rtp_stream_receiver.cc
diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc
index 88d86ef986714df85332333ef5cff9d6849bd1e0..320d1bd0798035d2375886455945b0464c9da74a 100644
--- a/webrtc/video/rtp_stream_receiver.cc
+++ b/webrtc/video/rtp_stream_receiver.cc
@@ -111,7 +111,8 @@ RtpStreamReceiver::RtpStreamReceiver(
                                     packet_router)),
       complete_frame_callback_(complete_frame_callback),
       keyframe_request_sender_(keyframe_request_sender),
-      timing_(timing) {
+      timing_(timing),
+      has_received_frame_(false) {
   packet_router_->AddReceiveRtpModule(rtp_rtcp_.get());
   rtp_receive_statistics_->RegisterRtpStatisticsCallback(receive_stats_proxy);
   rtp_receive_statistics_->RegisterRtcpStatisticsCallback(receive_stats_proxy);
@@ -380,6 +381,13 @@ int32_t RtpStreamReceiver::ResendPackets(const uint16_t* sequence_numbers,
 
 void RtpStreamReceiver::OnReceivedFrame(
     std::unique_ptr<video_coding::RtpFrameObject> frame) {
+
+  if (!has_received_frame_) {
+    has_received_frame_ = true;

[mflodman, 2017/05/11 13:11:36]

What happens if:
1. The frame buffer is reset later in a call, for some reason.
2. The first key frame is lost after the reset.

I guess that will lead to a 3 second freeze then?

This code will work for the first frame of a call, but not later in a call.
Right?

[philipel, 2017/05/11 13:38:58]

If things are working as they should (the incoming stream follow the spec on the
most basic level) the frame buffer should never be reset, the reseting of the
frame buffer is another hack to allow random jumps in the stream.
This hack is to minimize the problem of recreating a receive stream while
receiving a stream. I'm not sure what you mean by reset in this case, I thought
recreating was the only way.
Exactly, we only do this check for the first frame.

[mflodman, 2017/05/15 08:46:29]

I was thinking about a jitter buffer flush, or the equivalent in the new jitter
buffer. And I have to admit I don't know the details of how the new JB works and
maybe it's best to discuss this offline.

[stefan-webrtc, 2017/05/15 11:01:27]

Philip, I think the question is if the PacketBuffer, FrameBuffer or
RtpFrameReferenceFinder can be reset without us knowing it here, thus still
leading to a 3 second delay. I think whenever we have to reset the jitter buffer
in some way we should make sure we have a key frame as soon as possible. I'm
actually a bit surprised that this method is even called when we haven't
received a key frame. How can we have something decodable if that's the case?

BTW, this will send a single PLI. Maybe it's better to only set
has_received_frame_ to true if it's a key frame? That way if the RTCP is lost we
will try again.

[philipel, 2017/05/15 11:43:22]

This function is called when the packet buffer finds a complete frame, it has
yet to go through the reference finder and the frame buffer, so this function
being called before we have received a keyframe is not that strange (reordering,
recreating the video receive stream)
We will continuously retry to get a keyframe every 3000 ms if we don't have a
decodable stream, we could of course change that interval.

[stefan-webrtc, 2017/05/15 14:43:38]

Ah, I see.
Right, but every 3000 ms is a very low frequency. I'd expect it to be worth
sending way more often if it's a matter of actually getting the stream started.

Doesn't it make sense to set has_received_frame_ to false whenever the state of
the jitter buffer is reset? That's what I would've expected. Or is there some
other way we can get key frames more often until we have a decodable stream?
Maybe increase the frequency until we have something?

[philipel, 2017/05/16 11:26:23]

We might want to tweak how keyframe requests are sent before the first frame has
been decoded, but I don't think we should do that in this CL.

This CL was meant to implement a mechanism similar to that of the old  jitter
buffer (sending a keyframe request if the first frame sent for decoding isn't a
keyframe).

+    if (frame->FrameType() != kVideoFrameKey)
+      keyframe_request_sender_->RequestKeyFrame();

[brandtr, 2017/05/11 12:37:39]

Except for it being a heuristic, are there any drawbacks by doing this?

[philipel, 2017/05/11 13:38:58]

The worst drawback I can think of is on really bad networks where keyframes is a
big cost, and in the case of reordering this could potentially cause an extra
keyframe request. Other then that I can't think of any major drawback.

[stefan-webrtc, 2017/05/15 11:01:27]

Are these key frame requests throttled somewhere, or will this automatically
lead to an RTCP being sent?

[philipel, 2017/05/15 11:43:22]

Quickly looking through the code I can't find any throttle.

+  }
+
   if (!frame->delayed_by_retransmission())
     timing_->IncomingTimestamp(frame->timestamp, clock_->TimeInMilliseconds());
   reference_finder_->ManageFrame(std::move(frame));