Jitter delay now depend on protection mode (FEC/NACK).

Jitter delay now depend on protection mode (FEC/NACK).

R=stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/ae284089cc58eca51faa7f14ae0977e0f73a1f4f

[philipel, 2016-05-02 15:25:27]

philipel@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2016-05-04 09:30:48]

https://codereview.webrtc.org/1942683003/diff/1/webrtc/modules/video_coding/v...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/1942683003/diff/1/webrtc/modules/video_coding/v...
webrtc/modules/video_coding/video_receiver.cc:178: _receiver.SetNackMode(kNack,
media_optimization::kLowRttNackMs, 500);
500 should be a named constant. What does it represent? The rtt above which nack
is disabled? In that case I think -1 is correct, i.e., we never disable nack. Or
does it mean that we don't add an extra rtt above that threshold?

https://codereview.webrtc.org/1942683003/diff/1/webrtc/video/video_receive_st...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/1942683003/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:191:
vie_channel_.SetProtectionMode(config_.rtp.nack.rtp_history_ms > 0,
I think you may have to rebase this as mflodman changed this code recently (and
moved it into video_stream_decoder.cc, I think).

[philipel, 2016-05-04 10:56:05]

https://codereview.webrtc.org/1942683003/diff/1/webrtc/modules/video_coding/v...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/1942683003/diff/1/webrtc/modules/video_coding/v...
webrtc/modules/video_coding/video_receiver.cc:178: _receiver.SetNackMode(kNack,
media_optimization::kLowRttNackMs, 500);
On 2016/05/04 09:30:48, stefan-webrtc (holmer) wrote:
> 500 should be a named constant. What does it represent? The rtt above which
nack
> is disabled? In that case I think -1 is correct, i.e., we never disable nack.
Or
> does it mean that we don't add an extra rtt above that threshold?

This value represent the threshold rtt after which an rtt is no longer added to
the jitter buffer delay in the case of to many frames having at least on of
their packets nacked.

Maybe name it to something like "kMaxRttDelayThreshold"?

https://codereview.webrtc.org/1942683003/diff/1/webrtc/video/video_receive_st...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/1942683003/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:191:
vie_channel_.SetProtectionMode(config_.rtp.nack.rtp_history_ms > 0,
On 2016/05/04 09:30:48, stefan-webrtc (holmer) wrote:
> I think you may have to rebase this as mflodman changed this code recently
(and
> moved it into video_stream_decoder.cc, I think).

Talked to Magnus and there is still some refactoring in progress. I will wait
with rebasing until he is done.

[stefan-webrtc, 2016-05-06 11:23:12]

Let me know when the rebase is done.

[philipel, 2016-05-06 11:47:20]

On 2016/05/06 11:23:12, stefan-webrtc (holmer) wrote:
> Let me know when the rebase is done.

Waiting for https://codereview.webrtc.org/1947913002/ to land, shouldn't be
long.

[philipel, 2016-05-06 13:39:13]

Refactoring and rebasing is done, PTAL

[stefan-webrtc, 2016-05-06 14:11:47]

WDYT about making a future improvement where the added extra delay is a function
of how often we actually have to wait for a retransmission? That way, if we have
enough FEC recovery, the extra RTT will naturally not be added.

This is good enough for now though.

lgtm

https://codereview.webrtc.org/1942683003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/media_opt_util.h (right):

https://codereview.webrtc.org/1942683003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/media_opt_util.h:48: // If the RTT is higher than
this, then it wont be added to
If the RTT is higher than this an extra RTT wont be added to to the jitter
buffer delay.

https://codereview.webrtc.org/1942683003/diff/20001/webrtc/video/video_stream...
File webrtc/video/video_stream_decoder.cc (right):

https://codereview.webrtc.org/1942683003/diff/20001/webrtc/video/video_stream...
webrtc/video/video_stream_decoder.cc:61: else
Remove else and move line 62 to line 59

[philipel, 2016-05-06 15:07:30]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-05-06 15:07:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942683003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942683003/20001

[philipel, 2016-05-06 15:07:56]

The CQ bit was unchecked by philipel@webrtc.org

[philipel, 2016-05-06 15:12:11]

https://codereview.webrtc.org/1942683003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/media_opt_util.h (right):

https://codereview.webrtc.org/1942683003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/media_opt_util.h:48: // If the RTT is higher than
this, then it wont be added to
On 2016/05/06 14:11:46, stefan-webrtc (holmer) wrote:
> If the RTT is higher than this an extra RTT wont be added to to the jitter
> buffer delay.

Done.

https://codereview.webrtc.org/1942683003/diff/20001/webrtc/video/video_stream...
File webrtc/video/video_stream_decoder.cc (right):

https://codereview.webrtc.org/1942683003/diff/20001/webrtc/video/video_stream...
webrtc/video/video_stream_decoder.cc:61: else
On 2016/05/06 14:11:46, stefan-webrtc (holmer) wrote:
> Remove else and move line 62 to line 59

Hmm.. I think the way it is now is a clearer and easier to read.

[philipel, 2016-05-06 15:17:42]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-05-06 15:17:43]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1942683003/#ps40001 (title:
"Comment fix")

[commit-bot: I haz the power, 2016-05-06 15:17:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942683003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942683003/40001

[commit-bot: I haz the power, 2016-05-06 17:18:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-06 17:18:20]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[commit-bot: I haz the power, 2016-05-09 10:14:44]

Description was changed from

==========
Jitter delay now depend on protection mode (FEC/NACK).
==========

to

==========
Jitter delay now depend on protection mode (FEC/NACK).

R=stefan@webrtc.org

Committed: https://crrev.com/ae284089cc58eca51faa7f14ae0977e0f73a1f4f
Cr-Commit-Position: refs/heads/master@{#12661}
==========

[philipel, 2016-05-09 10:14:44]

Description was changed from

==========
Jitter delay now depend on protection mode (FEC/NACK).
==========

to

==========
Jitter delay now depend on protection mode (FEC/NACK).

R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/ae284089cc58eca51faa7f14a...
==========

[commit-bot: I haz the power, 2016-05-09 10:14:45]

Patchset 3 (id:??) landed as
https://crrev.com/ae284089cc58eca51faa7f14ae0977e0f73a1f4f
Cr-Commit-Position: refs/heads/master@{#12661}

[philipel, 2016-05-09 10:14:46]

Committed patchset #3 (id:40001) manually as
ae284089cc58eca51faa7f14ae0977e0f73a1f4f (presubmit successful).