Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2993793002:
Request keyframes more frequently on stream start/decoding error. (Closed)
DescriptionRequest 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
Patch Set 1 #
Total comments: 18
Messages
Total messages: 27 (9 generated)
philipel@webrtc.org changed reviewers: + stefan@webrtc.org, terelius@webrtc.org
https://codereview.webrtc.org/2993793002/diff/1/webrtc/modules/video_coding/v... File webrtc/modules/video_coding/video_receiver.cc (left): https://codereview.webrtc.org/2993793002/diff/1/webrtc/modules/video_coding/v... webrtc/modules/video_coding/video_receiver.cc:332: if (!frame.Complete() || frame.MissingFrame()) { I assume this if statement was used when we allowed decoding with artifacts.
The CQ bit was checked by philipel@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:509: keyframe_required_ = false; 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?
https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:509: keyframe_required_ = false; On 2017/08/07 11:54:38, stefan-webrtc wrote: > 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? 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).
https://codereview.webrtc.org/2993793002/diff/1/webrtc/modules/video_coding/v... File webrtc/modules/video_coding/video_receiver.cc (left): https://codereview.webrtc.org/2993793002/diff/1/webrtc/modules/video_coding/v... webrtc/modules/video_coding/video_receiver.cc:332: if (!frame.Complete() || frame.MissingFrame()) { On 2017/08/07 11:43:34, philipel wrote: > I assume this if statement was used when we allowed decoding with artifacts. I don't understand the purpose of this code. It would be good if someone who does could review the change. https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:494: static const int kMaxWaitForKeyFrameMs = 200; 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? https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:512: keyframe_required_ = true; Where do we actually send the keyframe request?
https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:494: static const int kMaxWaitForKeyFrameMs = 200; On 2017/08/07 12:00:06, terelius wrote: > 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? 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. https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:512: keyframe_required_ = true; On 2017/08/07 12:00:05, terelius wrote: > Where do we actually send the keyframe request? 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2993793002/diff/1/webrtc/modules/video_coding/v... File webrtc/modules/video_coding/video_receiver.cc (left): https://codereview.webrtc.org/2993793002/diff/1/webrtc/modules/video_coding/v... webrtc/modules/video_coding/video_receiver.cc:332: if (!frame.Complete() || frame.MissingFrame()) { On 2017/08/07 12:00:05, terelius wrote: > On 2017/08/07 11:43:34, philipel wrote: > > I assume this if statement was used when we allowed decoding with artifacts. > > I don't understand the purpose of this code. It would be good if someone who > does could review the change. I wrote it I think. It's purpose is to request a key frame if we've decoded a frame which is incomplete or misses references. That should no longer happen. https://codereview.webrtc.org/2993793002/diff/1/webrtc/modules/video_coding/v... webrtc/modules/video_coding/video_receiver.cc:338: _scheduleKeyRequest = true; Would be nice to get rid of this member eventually. https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:509: keyframe_required_ = false; On 2017/08/07 11:59:34, philipel wrote: > On 2017/08/07 11:54:38, stefan-webrtc wrote: > > 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? > > 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). Right, I missed that. Do we have test coverage for that?
lgtm https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:512: keyframe_required_ = true; On 2017/08/07 12:09:07, philipel wrote: > On 2017/08/07 12:00:05, terelius wrote: > > Where do we actually send the keyframe request? > > 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. Acknowledged.
https://codereview.webrtc.org/2993793002/diff/1/webrtc/modules/video_coding/v... File webrtc/modules/video_coding/video_receiver.cc (left): https://codereview.webrtc.org/2993793002/diff/1/webrtc/modules/video_coding/v... webrtc/modules/video_coding/video_receiver.cc:338: _scheduleKeyRequest = true; On 2017/08/07 12:24:53, stefan-webrtc wrote: > Would be nice to get rid of this member eventually. Yes, it was still used by other parts of this class, but it should be removed in some other cleanup CL. https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:509: keyframe_required_ = false; On 2017/08/07 12:24:53, stefan-webrtc wrote: > On 2017/08/07 11:59:34, philipel wrote: > > On 2017/08/07 11:54:38, stefan-webrtc wrote: > > > 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? > > > > 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). > > Right, I missed that. Do we have test coverage for that? 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
noahric@chromium.org changed reviewers: + noahric@chromium.org
https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:494: static const int kMaxWaitForKeyFrameMs = 200; On 2017/08/07 12:09:07, philipel wrote: > On 2017/08/07 12:00:06, terelius wrote: > > 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? > > 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. 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.
https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:494: static const int kMaxWaitForKeyFrameMs = 200; On 2017/08/07 15:12:29, noahric wrote: > On 2017/08/07 12:09:07, philipel wrote: > > On 2017/08/07 12:00:06, terelius wrote: > > > 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? > > > > 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. > > 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. 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.
https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:494: static const int kMaxWaitForKeyFrameMs = 200; On 2017/08/07 16:27:49, philipel wrote: > On 2017/08/07 15:12:29, noahric wrote: > > On 2017/08/07 12:09:07, philipel wrote: > > > On 2017/08/07 12:00:06, terelius wrote: > > > > 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? > > > > > > 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. > > > > 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. > > 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. > The old jitter buffer requested a new KF every 500 ms in the case of a decoding error. 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? > I know it's not as bad for video, but for screensharing sending an extra keyframe can be quite costly. 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?
On 2017/08/07 16:48:45, noahric wrote: > https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... > File webrtc/video/video_receive_stream.cc (right): > > https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... > webrtc/video/video_receive_stream.cc:494: static const int kMaxWaitForKeyFrameMs > = 200; > On 2017/08/07 16:27:49, philipel wrote: > > On 2017/08/07 15:12:29, noahric wrote: > > > On 2017/08/07 12:09:07, philipel wrote: > > > > On 2017/08/07 12:00:06, terelius wrote: > > > > > 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? > > > > > > > > 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. > > > > > > 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. > > > > 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. > > > The old jitter buffer requested a new KF every 500 ms in the case of a > decoding error. > 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? > > > I know it's not as bad for video, but for screensharing sending an extra > keyframe can be quite costly. > 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? To be clear, none of this would matter in my testing, since our rtt at the office is somewhere around 10-30ms :) Though I suppose that's really 1/2 rtt, since it's not end-to-end transmit delay.
On 2017/08/07 16:50:08, noahric wrote: > On 2017/08/07 16:48:45, noahric wrote: > > > https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... > > File webrtc/video/video_receive_stream.cc (right): > > > > > https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... > > webrtc/video/video_receive_stream.cc:494: static const int > kMaxWaitForKeyFrameMs > > = 200; > > On 2017/08/07 16:27:49, philipel wrote: > > > On 2017/08/07 15:12:29, noahric wrote: > > > > On 2017/08/07 12:09:07, philipel wrote: > > > > > On 2017/08/07 12:00:06, terelius wrote: > > > > > > 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? > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > > The old jitter buffer requested a new KF every 500 ms in the case of a > > decoding error. > > 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? > > > > > I know it's not as bad for video, but for screensharing sending an extra > > keyframe can be quite costly. > > 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? > > To be clear, none of this would matter in my testing, since our rtt at the > office is somewhere around 10-30ms :) Though I suppose that's really 1/2 rtt, > since it's not end-to-end transmit delay. I think it should be safe to request keyframes more often than once per rtt. The sender will throttle to not encode keyframes too often, and given that we send BWE feedback as often as once every 50 ms, it should be fine to at least send the keyframe requests once every 200 ms. I agree that for long rtts it may be good to send more often for redundancy.
lgtm https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:509: keyframe_required_ = false; On 2017/08/07 13:30:11, philipel wrote: > On 2017/08/07 12:24:53, stefan-webrtc wrote: > > On 2017/08/07 11:59:34, philipel wrote: > > > On 2017/08/07 11:54:38, stefan-webrtc wrote: > > > > 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? > > > > > > 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). > > > > Right, I missed that. Do we have test coverage for that? > > 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 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.
On 2017/08/09 07:02:36, stefan-webrtc wrote: > lgtm > > https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... > File webrtc/video/video_receive_stream.cc (right): > > https://codereview.webrtc.org/2993793002/diff/1/webrtc/video/video_receive_st... > webrtc/video/video_receive_stream.cc:509: keyframe_required_ = false; > On 2017/08/07 13:30:11, philipel wrote: > > On 2017/08/07 12:24:53, stefan-webrtc wrote: > > > On 2017/08/07 11:59:34, philipel wrote: > > > > On 2017/08/07 11:54:38, stefan-webrtc wrote: > > > > > 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? > > > > > > > > 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). > > > > > > Right, I missed that. Do we have test coverage for that? > > > > 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 > > 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. Created an issue for it: crbug.com/webrtc/8081.
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": 1502272496758280, "parent_rev":
"445f1a15359e75eaa308c8615e0310f9ae7d9d29", "commit_rev":
"26b48043581735eed6e36b95fae6f5b1bcf8cfb5"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/26b48043581735eed6e36b95f... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/26b48043581735eed6e36b95f...
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.webrtc.org/2994043002/ by deadbeef@webrtc.org. The reason for reverting is: 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()"?. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
