Make the new jitter buffer the default jitter buffer.

Make the new jitter buffer the default jitter buffer.

This CL contains only the changes necessary to make the switch to the new jitter
buffer, clean up will be done in follow up CLs.

In this CL:
 - Removed the WebRTC-NewVideoJitterBuffer experiment and made the
   new video jitter buffer the default one.
 - Moved WebRTC.Video.KeyFramesReceivedInPermille and
   WebRTC.Video.JitterBufferDelayInMs to the ReceiveStatisticsProxy.

BUG=webrtc:5514
Review-Url: https://codereview.webrtc.org/2627463004
Cr-Commit-Position: refs/heads/master@{#16114}
Committed: https://chromium.googlesource.com/external/webrtc/+/0f0763d86d5d4e7f27e8dece02560e39c6da97d6

[philipel, 2017-01-10 11:36:06]

Description was changed from

==========
Make the new jitter buffer the default jitter buffer.

This CL contains only the changes necessary to make the switch to the new jitter
buffer, clean up will be done in follow up CLs.

In this CL:
 - Removed the WebRTC-NewVideoJitterBuffer experiment and made the
   new video jitter buffer the default one.
 - Moved WebRTC.Video.KeyFramesReceivedInPermille and
   WebRTC.Video.JitterBufferDelayInMs to the ReceiveStatisticsProxy.

BUG:webrtc:5514
==========

to

==========
Make the new jitter buffer the default jitter buffer.

This CL contains only the changes necessary to make the switch to the new jitter
buffer, clean up will be done in follow up CLs.

In this CL:
 - Removed the WebRTC-NewVideoJitterBuffer experiment and made the
   new video jitter buffer the default one.
 - Moved WebRTC.Video.KeyFramesReceivedInPermille and
   WebRTC.Video.JitterBufferDelayInMs to the ReceiveStatisticsProxy.

BUG=webrtc:5514
==========

[philipel, 2017-01-10 11:48:16]

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

[philipel, 2017-01-10 11:48:16]

https://codereview.webrtc.org/2627463004/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2627463004/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:506: QualitySample();
Earlier, QualitySample was called from OnIncomingRate, which in turn was called
from VideoReceiver::Process about every second.

[philipel, 2017-01-11 16:12:34]

Just a correction about what I said earlier in our offline discussion: I don't
use the VideoStreamDecoder, ReceiveStatsProxy also implements
VCMReceiveStatisticsCallback.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:63:
_receiveStatsCallback->OnReceiveRatesUpdated(0, 0);
See ReceiveStatisticsProxy::OnIncomingRate why we do this callback.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:316: QualitySample();
This function is still called about every second from the VideoReceiver::Process
thread.

This is the work Viktor did to detect bad call quality and we don't use it yet,
so maybe we want to clean up even more in VideoReceiver::Process?

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.h:86: void OnRttUpdate(int64_t avg_rtt_ms,
int64_t max_rtt_ms) override;
Since I don't use the VideoStreamDecoder to pass information to the
ReceiveStatisticsProxy we no longer get the RTT as a parameter in the
OnDecoderTiming function.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.h:128: mutable VideoReceiveStream::Stats
stats_ GUARDED_BY(crit_);
Since GetStats is marked as const and we want to update the framerate/bitrate as
we call that function, we make |stats_| mutable in order to be able to modify it
anyway.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:248: clock_, jitter_estimator_.get(),
timing_.get(), &stats_proxy_));
I don't use the VideoStreamDecoder, ReceiveStatsProxy also implements
VCMReceiveStatisticsCallback.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/video_stream...
File webrtc/video/video_stream_decoder.h (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/video_stream...
webrtc/video/video_stream_decoder.h:72: void OnCompleteFrame(bool is_keyframe,
size_t size_bytes) override;
Since this function is pure virtual we have to implement it.

[terelius, 2017-01-11 16:15:22]

Do you have further changes pending or should we start reviewing?

[philipel, 2017-01-11 16:16:07]

On 2017/01/11 16:15:22, terelius wrote:
> Do you have further changes pending or should we start reviewing?

You can start reviewing :)

[terelius, 2017-01-12 13:08:34]

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy_unittest.cc (left):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy_unittest.cc:77:
TEST_F(ReceiveStatisticsProxyTest, GetStatsReportsIncomingRate) {
Is this no longer relevant, or is there some other test for it?

[stefan-webrtc, 2017-01-12 14:16:16]

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

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:160: VCMReceiveStatisticsCallback*
const stats_callback_ GUARDED_BY(crit_);
Why protected by crit_?

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2_unittest.cc:98: buffer_(&clock_,
&jitter_estimator_, &timing_, nullptr),
Perhaps add a simple test for this instead.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_coding_defines.h:96: virtual void
OnDecoderTiming(int decode_ms,
OnDecoderTimingUpdated

Not sure about "Decoder" in the name... OnBufferTimingUpdated?

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:63:
_receiveStatsCallback->OnReceiveRatesUpdated(0, 0);
On 2017/01/11 16:12:34, philipel wrote:
> See ReceiveStatisticsProxy::OnIncomingRate why we do this callback.

I don't understand the question? We want to notify the stats proxy about the
bitrate and framerate received on this stream, I guess?

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (left):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:1193: 
As mentioned offline, I'm not sure we want to delete this. We should verify that
we're requesting a key frame (PLI) after some time if the stream doesn't become
decodable.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:316: QualitySample();
On 2017/01/11 16:12:34, philipel wrote:
> This function is still called about every second from the
VideoReceiver::Process
> thread.
> 
> This is the work Viktor did to detect bad call quality and we don't use it
yet,
> so maybe we want to clean up even more in VideoReceiver::Process?

Ouch... Why is it called here? Can we move it to somewhere else where we have
periodic calls? Or just add a todo to clean up

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy_unittest.cc (left):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy_unittest.cc:80:
statistics_proxy_->OnIncomingRate(kFramerate, kBitrateBps);
Perhaps replace this with calls to OnCompleteFrame() so that we still can verify
that stats calculations work?

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:203: new NackModule(clock_, nack_sender,
keyframe_request_sender));
Move to initializer list if possible.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:206: packet_buffer_ =
video_coding::PacketBuffer::Create(
Same here.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:208: reference_finder_.reset(new
video_coding::RtpFrameReferenceFinder(this));
and here.

[philipel, 2017-01-12 16:41:35]

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

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:160: VCMReceiveStatisticsCallback*
const stats_callback_ GUARDED_BY(crit_);
On 2017/01/12 14:16:15, stefan-webrtc wrote:
> Why protected by crit_?

No reason, removed protection.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2_unittest.cc:98: buffer_(&clock_,
&jitter_estimator_, &timing_, nullptr),
On 2017/01/12 14:16:15, stefan-webrtc wrote:
> Perhaps add a simple test for this instead.

Done.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_coding_defines.h:96: virtual void
OnDecoderTiming(int decode_ms,
On 2017/01/12 14:16:15, stefan-webrtc wrote:
> OnDecoderTimingUpdated
> 
> Not sure about "Decoder" in the name... OnBufferTimingUpdated?

Changed to OnFrameBufferTimingsUpdated.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:63:
_receiveStatsCallback->OnReceiveRatesUpdated(0, 0);
On 2017/01/12 14:16:15, stefan-webrtc wrote:
> On 2017/01/11 16:12:34, philipel wrote:
> > See ReceiveStatisticsProxy::OnIncomingRate why we do this callback.
> 
> I don't understand the question? We want to notify the stats proxy about the
> bitrate and framerate received on this stream, I guess?

Yes, but this since the old jitter buffer wont be used any more this wont give
us anything meaningful. The only reason I kept this piece of code is so that
QualitySample (Viktors work) is called as with the old jitter buffer.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (left):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:1193: 
On 2017/01/12 14:16:15, stefan-webrtc wrote:
> As mentioned offline, I'm not sure we want to delete this. We should verify
that
> we're requesting a key frame (PLI) after some time if the stream doesn't
become
> decodable.

Fixed the NackModule to not send NACKs when history is 0.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:316: QualitySample();
On 2017/01/12 14:16:15, stefan-webrtc wrote:
> On 2017/01/11 16:12:34, philipel wrote:
> > This function is still called about every second from the
> VideoReceiver::Process
> > thread.
> > 
> > This is the work Viktor did to detect bad call quality and we don't use it
> yet,
> > so maybe we want to clean up even more in VideoReceiver::Process?
> 
> Ouch... Why is it called here? Can we move it to somewhere else where we have
> periodic calls? Or just add a todo to clean up

I will take a look at it in one of my clean up CLs.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy_unittest.cc (left):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy_unittest.cc:77:
TEST_F(ReceiveStatisticsProxyTest, GetStatsReportsIncomingRate) {
On 2017/01/12 13:08:34, terelius wrote:
> Is this no longer relevant, or is there some other test for it?

Since the old jitter buffer wont be used anymore no meaningful data will be
reported with the OnIncomingRate function. To avoid it overwriting the actual
data (that is being reported with the new OnCompleteFrame function) I removed
all logic (almost) from OnIncomingRate.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy_unittest.cc:80:
statistics_proxy_->OnIncomingRate(kFramerate, kBitrateBps);
On 2017/01/12 14:16:15, stefan-webrtc wrote:
> Perhaps replace this with calls to OnCompleteFrame() so that we still can
verify
> that stats calculations work?

Done.

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2627463004/diff/40001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:203: new NackModule(clock_, nack_sender,
keyframe_request_sender));
On 2017/01/12 14:16:16, stefan-webrtc wrote:
> Move to initializer list if possible.

I think it's better to do clean up in followup CLs.

[stefan-webrtc, 2017-01-13 09:10:59]

lgtm

https://codereview.webrtc.org/2627463004/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/2627463004/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:63:
_receiveStatsCallback->OnReceiveRatesUpdated(0, 0);
Please add a todo to clean this up. Maybe the QualitySample() should be called
on OnCompleteFrame() instead?

[philipel, 2017-01-13 09:19:01]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-01-13 09:19:02]

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/2627463004/#ps80001 (title:
"Added TODO.")

[commit-bot: I haz the power, 2017-01-13 09:19:07]

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

[commit-bot: I haz the power, 2017-01-13 09:37:02]

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

[commit-bot: I haz the power, 2017-01-13 09:37:03]

Try jobs failed on following builders:
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/21024)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/21642)

[philipel, 2017-01-13 13:43:20]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-01-13 13:43:21]

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/2627463004/#ps100001 (title:
"Initialize frame_window_accumulated_bytes_ + static_casts.")

[commit-bot: I haz the power, 2017-01-13 13:43:26]

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

[commit-bot: I haz the power, 2017-01-13 14:02:39]

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

[commit-bot: I haz the power, 2017-01-13 14:02:40]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[philipel, 2017-01-13 14:21:25]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-01-13 14:21:26]

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/2627463004/#ps120001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-01-13 14:21:35]

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

[commit-bot: I haz the power, 2017-01-13 14:45:24]

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

[commit-bot: I haz the power, 2017-01-13 14:45:25]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[philipel, 2017-01-16 15:18:12]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2017-01-16 15:18:21]

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

[commit-bot: I haz the power, 2017-01-16 16:44:53]

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

[commit-bot: I haz the power, 2017-01-16 16:44:54]

Try jobs failed on following builders:
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/20142)

[philipel, 2017-01-17 11:28:10]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2017-01-17 11:28:14]

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

[commit-bot: I haz the power, 2017-01-17 11:31:15]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484652490588650,
"parent_rev": "804ab6f27f7ee0ea4314110dc6e89885ed4f290d", "commit_rev":
"0f0763d86d5d4e7f27e8dece02560e39c6da97d6"}

[commit-bot: I haz the power, 2017-01-17 11:31:18]

Description was changed from

==========
Make the new jitter buffer the default jitter buffer.

This CL contains only the changes necessary to make the switch to the new jitter
buffer, clean up will be done in follow up CLs.

In this CL:
 - Removed the WebRTC-NewVideoJitterBuffer experiment and made the
   new video jitter buffer the default one.
 - Moved WebRTC.Video.KeyFramesReceivedInPermille and
   WebRTC.Video.JitterBufferDelayInMs to the ReceiveStatisticsProxy.

BUG=webrtc:5514
==========

to

==========
Make the new jitter buffer the default jitter buffer.

This CL contains only the changes necessary to make the switch to the new jitter
buffer, clean up will be done in follow up CLs.

In this CL:
 - Removed the WebRTC-NewVideoJitterBuffer experiment and made the
   new video jitter buffer the default one.
 - Moved WebRTC.Video.KeyFramesReceivedInPermille and
   WebRTC.Video.JitterBufferDelayInMs to the ReceiveStatisticsProxy.

BUG=webrtc:5514

Review-Url: https://codereview.webrtc.org/2627463004
Cr-Commit-Position: refs/heads/master@{#16114}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0f0763d86d5d4e7f27e8dece0...
==========

[commit-bot: I haz the power, 2017-01-17 11:31:19]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/0f0763d86d5d4e7f27e8dece0...

[philipel, 2017-01-17 12:03:38]

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

The reason for reverting is: Breaks android bots..