Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874933004:
Request keyframe if the first received frame is not a keyframe. (Closed)
DescriptionRequest keyframe if the first received frame is not a keyframe.
BUG=webrtc:7669
Review-Url: https://codereview.webrtc.org/2874933004
Cr-Commit-Position: refs/heads/master@{#18169}
Committed: https://chromium.googlesource.com/external/webrtc/+/2c53b13a3be7a0f6cac5e4e89e3b1624031ad2dc
Patch Set 1 #
Total comments: 11
Messages
Total messages: 21 (6 generated)
philipel@webrtc.org changed reviewers: + brandtr@webrtc.org, mflodman@webrtc.org
PTAL
lgtm. This change significantly reduces the delay after a VideoReceiveStream recreation, caused by a SDP renegotiation. There is still a delay, but I believe that is caused by the decoder reinit time + the time for the keyframe to arrive. https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:388: keyframe_request_sender_->RequestKeyFrame(); Except for it being a heuristic, are there any drawbacks by doing this?
On 2017/05/11 12:37:39, brandtr wrote: > lgtm. > > This change significantly reduces the delay after a VideoReceiveStream > recreation, caused by a SDP renegotiation. There is still a delay, but I believe > that is caused by the decoder reinit time + the time for the keyframe to arrive. > > https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... > File webrtc/video/rtp_stream_receiver.cc (right): > > https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... > webrtc/video/rtp_stream_receiver.cc:388: > keyframe_request_sender_->RequestKeyFrame(); > Except for it being a heuristic, are there any drawbacks by doing this? Maybe add a bug, so this change can be tracked?
https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:386: has_received_frame_ = true; 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?
https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:386: has_received_frame_ = true; On 2017/05/11 13:11:36, mflodman wrote: > What happens if: > 1. The frame buffer is reset later in a call, for some reason. 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. > 2. The first key frame is lost after the reset. > > I guess that will lead to a 3 second freeze then? 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. > > This code will work for the first frame of a call, but not later in a call. > Right? Exactly, we only do this check for the first frame. https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:388: keyframe_request_sender_->RequestKeyFrame(); On 2017/05/11 12:37:39, brandtr wrote: > Except for it being a heuristic, are there any drawbacks by doing this? 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.
mflodman@webrtc.org changed reviewers: + stefan@webrtc.org
Adding Stefan as a reviewer, since he knows more about the new JB than Rasmus and I do, and how this compares to the old one. https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:386: has_received_frame_ = true; On 2017/05/11 13:38:58, philipel wrote: > On 2017/05/11 13:11:36, mflodman wrote: > > What happens if: > > 1. The frame buffer is reset later in a call, for some reason. > > 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. > > > 2. The first key frame is lost after the reset. > > > > I guess that will lead to a 3 second freeze then? > > 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. 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. > > > > This code will work for the first frame of a call, but not later in a call. > > Right? > > Exactly, we only do this check for the first frame. > > >
On 2017/05/11 12:38:48, brandtr wrote: > On 2017/05/11 12:37:39, brandtr wrote: > > lgtm. > > > > This change significantly reduces the delay after a VideoReceiveStream > > recreation, caused by a SDP renegotiation. There is still a delay, but I > believe > > that is caused by the decoder reinit time + the time for the keyframe to > arrive. > > > > > https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... > > File webrtc/video/rtp_stream_receiver.cc (right): > > > > > https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... > > webrtc/video/rtp_stream_receiver.cc:388: > > keyframe_request_sender_->RequestKeyFrame(); > > Except for it being a heuristic, are there any drawbacks by doing this? > > Maybe add a bug, so this change can be tracked? Yes, a bug for this would be good.
https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:386: has_received_frame_ = true; On 2017/05/15 08:46:29, mflodman wrote: > On 2017/05/11 13:38:58, philipel wrote: > > On 2017/05/11 13:11:36, mflodman wrote: > > > What happens if: > > > 1. The frame buffer is reset later in a call, for some reason. > > > > 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. > > > > > 2. The first key frame is lost after the reset. > > > > > > I guess that will lead to a 3 second freeze then? > > > > 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. > 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. > > > > > > > > This code will work for the first frame of a call, but not later in a call. > > > Right? > > > > Exactly, we only do this check for the first frame. > > > > > > > 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. https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:388: keyframe_request_sender_->RequestKeyFrame(); On 2017/05/11 13:38:58, philipel wrote: > On 2017/05/11 12:37:39, brandtr wrote: > > Except for it being a heuristic, are there any drawbacks by doing this? > > 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. Are these key frame requests throttled somewhere, or will this automatically lead to an RTCP being sent?
https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:386: has_received_frame_ = true; On 2017/05/15 11:01:27, stefan-webrtc wrote: > On 2017/05/15 08:46:29, mflodman wrote: > > On 2017/05/11 13:38:58, philipel wrote: > > > On 2017/05/11 13:11:36, mflodman wrote: > > > > What happens if: > > > > 1. The frame buffer is reset later in a call, for some reason. > > > > > > 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. > > > > > > > 2. The first key frame is lost after the reset. > > > > > > > > I guess that will lead to a 3 second freeze then? > > > > > > 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. > > 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. > > > > > > > > > > > > This code will work for the first frame of a call, but not later in a > call. > > > > Right? > > > > > > Exactly, we only do this check for the first frame. > > > > > > > > > > > > > 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? 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) > 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. 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. https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:388: keyframe_request_sender_->RequestKeyFrame(); On 2017/05/15 11:01:27, stefan-webrtc wrote: > On 2017/05/11 13:38:58, philipel wrote: > > On 2017/05/11 12:37:39, brandtr wrote: > > > Except for it being a heuristic, are there any drawbacks by doing this? > > > > 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. > > Are these key frame requests throttled somewhere, or will this automatically > lead to an RTCP being sent? Quickly looking through the code I can't find any throttle.
https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:386: has_received_frame_ = true; On 2017/05/15 11:43:22, philipel wrote: > On 2017/05/15 11:01:27, stefan-webrtc wrote: > > On 2017/05/15 08:46:29, mflodman wrote: > > > On 2017/05/11 13:38:58, philipel wrote: > > > > On 2017/05/11 13:11:36, mflodman wrote: > > > > > What happens if: > > > > > 1. The frame buffer is reset later in a call, for some reason. > > > > > > > > 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. > > > > > > > > > 2. The first key frame is lost after the reset. > > > > > > > > > > I guess that will lead to a 3 second freeze then? > > > > > > > > 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. > > > 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. > > > > > > > > > > > > > > > > This code will work for the first frame of a call, but not later in a > > call. > > > > > Right? > > > > > > > > Exactly, we only do this check for the first frame. > > > > > > > > > > > > > > > > > > > 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? > > 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) Ah, I see. > > > 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. > > 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. > 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?
https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2874933004/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:386: has_received_frame_ = true; On 2017/05/15 14:43:38, stefan-webrtc wrote: > On 2017/05/15 11:43:22, philipel wrote: > > On 2017/05/15 11:01:27, stefan-webrtc wrote: > > > On 2017/05/15 08:46:29, mflodman wrote: > > > > On 2017/05/11 13:38:58, philipel wrote: > > > > > On 2017/05/11 13:11:36, mflodman wrote: > > > > > > What happens if: > > > > > > 1. The frame buffer is reset later in a call, for some reason. > > > > > > > > > > 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. > > > > > > > > > > > 2. The first key frame is lost after the reset. > > > > > > > > > > > > I guess that will lead to a 3 second freeze then? > > > > > > > > > > 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. > > > > 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. > > > > > > > > > > > > > > > > > > > > This code will work for the first frame of a call, but not later in a > > > call. > > > > > > Right? > > > > > > > > > > Exactly, we only do this check for the first frame. > > > > > > > > > > > > > > > > > > > > > > > > > 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? > > > > 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) > > Ah, I see. > > > > > > 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. > > > > 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. > > > > 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? 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).
lgtm I agree that it's probably better to get something in now that solves the immediate problem, and then we can discuss separately if we need some other handling of the more general problem.
Please refer to a bug as already requested.
Description was changed from ========== Request keyframe if the first received frame is not a keyframe. BUG=None ========== to ========== Request keyframe if the first received frame is not a keyframe. BUG=webrtc:7669 ==========
The CQ bit was checked by philipel@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1494944744177830, "parent_rev":
"94f6fa0526c2bbb5c953efad752a0d6658f2c81e", "commit_rev":
"2c53b13a3be7a0f6cac5e4e89e3b1624031ad2dc"}
Message was sent while issue was closed.
Description was changed from ========== Request keyframe if the first received frame is not a keyframe. BUG=webrtc:7669 ========== to ========== Request keyframe if the first received frame is not a keyframe. BUG=webrtc:7669 Review-Url: https://codereview.webrtc.org/2874933004 Cr-Commit-Position: refs/heads/master@{#18169} Committed: https://chromium.googlesource.com/external/webrtc/+/2c53b13a3be7a0f6cac5e4e89... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/2c53b13a3be7a0f6cac5e4e89... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
