Fix the video buffer size should take rtt into consideration

Fix the video buffer size should take rtt into consideration

BUG=webrtc:8010
Review-Url: https://codereview.webrtc.org/2980413002
Cr-Commit-Position: refs/heads/master@{#19285}
Committed: https://chromium.googlesource.com/external/webrtc/+/f1e08d0b5848d32fd31c5b6e4e570115c32b7ce5

[ggarber, 2017-07-19 15:20:49]

Description was changed from

==========
Fix the video buffer size should take rtt into consideration

BUG=webrtc:8010
==========

to

==========
Fix the video buffer size should take rtt into consideration

BUG=webrtc:8010
==========

[ggarber, 2017-07-19 15:20:50]

gustavogb@gmail.com changed reviewers:
+ philipel@webrtc.org

[philipel2, 2017-07-24 14:17:22]

philipel@chromium.org changed reviewers:
+ philipel@chromium.org

[philipel2, 2017-07-24 14:17:23]

Looks good, a few comment though :)

https://codereview.webrtc.org/2980413002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2980413002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:278:
jitter_estimator_->FrameNacked();
I think we should move this inside the if block on line 131.

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

https://codereview.webrtc.org/2980413002/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:440:
jitter_estimator_->UpdateRtt(avg_rtt_ms);
We shouldn't really update the jitter estimator outside the frame buffer (we
create and pass in the jitter estimator for the sake of testability). I think
implementing an UpdateRtt(int64_t rtt_ms) function in the FrameBuffer which in
turn makes this call is better.

Also, we should use the max_rtt_ms instead of the avg_rtt_ms.

[philipel2, 2017-07-24 14:19:07]

philipel@chromium.org changed reviewers:
- philipel@chromium.org

[philipel2, 2017-07-24 14:19:39]

Add ggarber to cc.

[ggarber, 2017-07-28 16:06:27]

On 2017/07/24 14:19:39, philipel2 wrote:
> Add ggarber to cc.

Thx a lot @philipel2.   Hopefully the new changes address all your comments. 
Let me know if anything else is needed

[philipel, 2017-07-28 16:11:44]

lg, just some nits this time.

https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:247: void
FrameBuffer::UpdateRtt(int64_t rttMs) {
rttMs --> rtt_ms

https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:249:
jitter_estimator_->UpdateRtt(rttMs);
rttMs --> rtt_ms

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

https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:76: // Updates the RTT for jitter
buffer estimation.
Just comment with "Updates the RTT for jitter buffer estimation." and skip the
rest.

https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:80: void UpdateRtt(int64_t rttMs);
rttMs --> rtt_ms

[ggarber, 2017-08-09 10:06:41]

On 2017/07/28 16:11:44, philipel wrote:
> lg, just some nits this time.
> 
>
https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
> File webrtc/modules/video_coding/frame_buffer2.cc (right):
> 
>
https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/frame_buffer2.cc:247: void
> FrameBuffer::UpdateRtt(int64_t rttMs) {
> rttMs --> rtt_ms
> 
>
https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/frame_buffer2.cc:249:
> jitter_estimator_->UpdateRtt(rttMs);
> rttMs --> rtt_ms
> 
>
https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
> File webrtc/modules/video_coding/frame_buffer2.h (right):
> 
>
https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/frame_buffer2.h:76: // Updates the RTT for jitter
> buffer estimation.
> Just comment with "Updates the RTT for jitter buffer estimation." and skip the
> rest.
> 
>
https://codereview.webrtc.org/2980413002/diff/20001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/frame_buffer2.h:80: void UpdateRtt(int64_t rttMs);
> rttMs --> rtt_ms

@Philipel, I made the changes you suggested some days ago.  Is there anything
else missing?

[philipel, 2017-08-09 11:21:42]

lgtm

[philipel, 2017-08-09 11:21:51]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2017-08-09 11:21:57]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[philipel, 2017-08-09 11:23:54]

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

[philipel, 2017-08-09 11:23:56]

Erik, could we get a rs-lgtm for webrtc/video/* please.

[sprang_webrtc, 2017-08-09 11:26:14]

lgtm

[commit-bot: I haz the power, 2017-08-09 12:43:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1502277711239580,
"parent_rev": "5ca60cc91cb21d4eded8b7ee401ee0b6cf280a2c", "commit_rev":
"f1e08d0b5848d32fd31c5b6e4e570115c32b7ce5"}

[commit-bot: I haz the power, 2017-08-09 12:43:15]

Description was changed from

==========
Fix the video buffer size should take rtt into consideration

BUG=webrtc:8010
==========

to

==========
Fix the video buffer size should take rtt into consideration

BUG=webrtc:8010

Review-Url: https://codereview.webrtc.org/2980413002
Cr-Commit-Position: refs/heads/master@{#19285}
Committed:
https://chromium.googlesource.com/external/webrtc/+/f1e08d0b5848d32fd31c5b6e4...
==========

[commit-bot: I haz the power, 2017-08-09 12:43:17]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/f1e08d0b5848d32fd31c5b6e4...

[philipel, 2017-08-21 12:25:30]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/3002033002/ by philipel@webrtc.org.

The reason for reverting is: We are not certain this is behavior we want..

[ggarber, 2017-08-21 13:39:14]

On 2017/08/21 12:25:30, philipel wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/3002033002/ by mailto:philipel@webrtc.org.
> 
> The reason for reverting is: We are not certain this is behavior we want..

Thx philipel.   

- Wasn't this the behaviour before the video buffer was rewritten.
- How are retransmissions going to be received in time if you don't wait for rtt
msecs?