Remove ViEEncoder::SetNetworkStatus

Remove ViEEncoder::SetNetworkStatus

This cl removed ViEEncoder::SetNetworkStatus. Instead the PacedSender will report that frames can not be sent when the network is down and the BitrateController will report an estimated available bandwidth of 0 bps.

BUG=webrtc:5687
NOTRY=True

Committed: https://crrev.com/50b5c3be844ef571a28b2681c549443a26735d72
Cr-Commit-Position: refs/heads/master@{#12699}

[perkj_webrtc, 2016-04-28 13:49:05]

Description was changed from

==========
Remove ViESender::SetNetworkStatus

BUG=
==========

to

==========
Remove ViEEncoder::SetNetworkStatus

This cl removed ViEEncoder::SetNetworkStatus. Instead the PacedSender will
report that frames can not be sent when the network is down and the
BitrateController will report an estimated available bandwidth of 0 bps.

BUG=webrtc:5687
==========

[perkj_webrtc, 2016-04-28 13:49:38]

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

[perkj_webrtc, 2016-04-28 13:49:38]

Can you please review?

[stefan-webrtc, 2016-05-02 10:56:31]

https://codereview.webrtc.org/1932683002/diff/40001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/1932683002/diff/40001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:333: return !paused_ &&
ExpectedQueueTimeMs() < PacedSender::kMaxQueueLengthMs;
This method takes the lock twice, once on line 332 and once on line 337.

https://codereview.webrtc.org/1932683002/diff/40001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.h (right):

https://codereview.webrtc.org/1932683002/diff/40001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.h:120: int64_t ExpectedQueueTimeMs() const;
Can this be made private?

https://codereview.webrtc.org/1932683002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/remb.h (right):

https://codereview.webrtc.org/1932683002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/remb.h:18: #include
"webrtc/modules/pacing//paced_sender.h"
remove one /

https://codereview.webrtc.org/1932683002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc
(right):

https://codereview.webrtc.org/1932683002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc:26:
BitrateController::CreateBitrateController(clock, observer, &pacer_)),
This isn't the correct pacer if a PacedVideoSender is used. Take a look at class
PacedVideoSender in packet_sender.h/.cc

https://codereview.webrtc.org/1932683002/diff/40001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1932683002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:722: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_)
{}

[perkj_webrtc, 2016-05-03 09:03:17]

PTAL

[stefan-webrtc, 2016-05-04 07:28:58]

https://codereview.webrtc.org/1932683002/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/1932683002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:301:
UpdateSendQueueStatus(send_queue_is_full)) {
I don't like this if statement. Can we do something to make it easier to read?
Maybe move out UpdateSendQueueStatus()? It's not even clear to me what that
method would return based on just reading the if statement.

[stefan-webrtc, 2016-05-04 09:21:20]

Nice! LGTM with the nit fixed.

https://codereview.webrtc.org/1932683002/diff/100001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/1932683002/diff/100001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/congestion_controller.cc:296: bool
CongestionController::HasBitrateToReportChanged(uint32_t bitrate_bps) {
Empty line above.

[perkj_webrtc, 2016-05-04 10:00:16]

https://codereview.webrtc.org/1932683002/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/1932683002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:301:
UpdateSendQueueStatus(send_queue_is_full)) {
On 2016/05/04 07:28:58, stefan-webrtc (holmer) wrote:
> I don't like this if statement. Can we do something to make it easier to read?
> Maybe move out UpdateSendQueueStatus()? It's not even clear to me what that
> method would return based on just reading the if statement.

I agree - and its not even 100% correct. PTAL

https://codereview.webrtc.org/1932683002/diff/100001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/1932683002/diff/100001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/congestion_controller.cc:296: bool
CongestionController::HasBitrateToReportChanged(uint32_t bitrate_bps) {
On 2016/05/04 09:21:20, stefan-webrtc (holmer) wrote:
> Empty line above.

Done.

[stefan-webrtc, 2016-05-04 12:30:51]

lgtm

[perkj_webrtc, 2016-05-04 15:25:14]

The CQ bit was checked by perkj@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-04 15:25:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1932683002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1932683002/140001

[commit-bot: I haz the power, 2016-05-04 15:30:33]

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

[commit-bot: I haz the power, 2016-05-04 15:30:34]

Dry run: Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5301)

[perkj_webrtc, 2016-05-12 07:25:16]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-05-12 07:25:16]

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/1932683002/#ps200001 (title:
"Rebased and fixed CongestionController unittests.")

[commit-bot: I haz the power, 2016-05-12 07:25:30]

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

[commit-bot: I haz the power, 2016-05-12 07:31:17]

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

[commit-bot: I haz the power, 2016-05-12 07:31:18]

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

[perkj_webrtc, 2016-05-12 11:42:31]

Description was changed from

==========
Remove ViEEncoder::SetNetworkStatus

This cl removed ViEEncoder::SetNetworkStatus. Instead the PacedSender will
report that frames can not be sent when the network is down and the
BitrateController will report an estimated available bandwidth of 0 bps.

BUG=webrtc:5687
==========

to

==========
Remove ViEEncoder::SetNetworkStatus

This cl removed ViEEncoder::SetNetworkStatus. Instead the PacedSender will
report that frames can not be sent when the network is down and the
BitrateController will report an estimated available bandwidth of 0 bps.

BUG=webrtc:5687
NOTRY=True
==========

[perkj_webrtc, 2016-05-12 11:42:41]

The CQ bit was checked by perkj@webrtc.org

[commit-bot: I haz the power, 2016-05-12 11:42:51]

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

[commit-bot: I haz the power, 2016-05-12 11:53:50]

Description was changed from

==========
Remove ViEEncoder::SetNetworkStatus

This cl removed ViEEncoder::SetNetworkStatus. Instead the PacedSender will
report that frames can not be sent when the network is down and the
BitrateController will report an estimated available bandwidth of 0 bps.

BUG=webrtc:5687
NOTRY=True
==========

to

==========
Remove ViEEncoder::SetNetworkStatus

This cl removed ViEEncoder::SetNetworkStatus. Instead the PacedSender will
report that frames can not be sent when the network is down and the
BitrateController will report an estimated available bandwidth of 0 bps.

BUG=webrtc:5687
NOTRY=True
==========

[commit-bot: I haz the power, 2016-05-12 11:53:52]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-12 11:53:56]

Description was changed from

==========
Remove ViEEncoder::SetNetworkStatus

This cl removed ViEEncoder::SetNetworkStatus. Instead the PacedSender will
report that frames can not be sent when the network is down and the
BitrateController will report an estimated available bandwidth of 0 bps.

BUG=webrtc:5687
NOTRY=True
==========

to

==========
Remove ViEEncoder::SetNetworkStatus

This cl removed ViEEncoder::SetNetworkStatus. Instead the PacedSender will
report that frames can not be sent when the network is down and the
BitrateController will report an estimated available bandwidth of 0 bps.

BUG=webrtc:5687
NOTRY=True

Committed: https://crrev.com/50b5c3be844ef571a28b2681c549443a26735d72
Cr-Commit-Position: refs/heads/master@{#12699}
==========

[commit-bot: I haz the power, 2016-05-12 11:53:57]

Patchset 11 (id:??) landed as
https://crrev.com/50b5c3be844ef571a28b2681c549443a26735d72
Cr-Commit-Position: refs/heads/master@{#12699}

[perkj_webrtc, 2016-05-13 08:17:20]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.webrtc.org/1978783002/ by perkj@webrtc.org.

The reason for reverting is: Breaks Chrome FYI using H264. 
Need to investigate. 

https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win7%20Tester/build....