Clean up of CongestionController.

Clean up of CongestionController.

Removes unused methods and moves out ViERemb to Call.

R=pbos@webrtc.org, solenberg@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/58c664c13dee0ed39a4d34683da5cf288455c0e5

[stefan-webrtc, 2016-02-04 12:37:14]

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

[stefan-webrtc, 2016-02-04 12:37:14]

I feel like I've accumulated a few clean up TODOs the last couple of CLs. This
is one of them I believe. :)

[stefan-webrtc, 2016-02-04 12:38:46]

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

[stefan-webrtc, 2016-02-04 12:38:46]

Adding pbos for the video parts of this CL.

[pbos-webrtc, 2016-02-04 13:53:20]

lgtm for webrtc/video with this fixed.

https://codereview.webrtc.org/1663413003/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1663413003/diff/1/webrtc/call/call.cc#newcode169
webrtc/call/call.cc:169: rtc::scoped_ptr<VieRemb> remb_;
VieRemb remb_, don't allocate on the heap here.

[stefan-webrtc, 2016-02-05 12:58:15]

Comments addressed.

[stefan-webrtc, 2016-02-05 12:58:52]

https://codereview.webrtc.org/1663413003/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1663413003/diff/1/webrtc/call/call.cc#newcode169
webrtc/call/call.cc:169: rtc::scoped_ptr<VieRemb> remb_;
On 2016/02/04 13:53:20, pbos-webrtc wrote:
> VieRemb remb_, don't allocate on the heap here.

Done.

[the sun, 2016-02-05 15:12:37]

Nice cleanup!

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/call/congestion_co...
File webrtc/call/congestion_controller.cc (right):

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/call/congestion_co...
webrtc/call/congestion_controller.cc:146: Clock* clock,
Store clock in a field and use instead of GetRealTimeClock throughout this file?

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_estimator.h
(right):

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_estimator.h:25:
unsigned int bitrate));
do we want this to be a uint32_t instead?

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

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:156: remb_(remb) {
RTC_DCHECK(remb);
wouldn't hurt to be paranoid about the other fields as well; fail early.

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

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:134: use_config_bitrate_(true) {
Same as in receive stream, RTC_DCHECK(remb); and the others

[stefan-webrtc, 2016-02-07 16:50:01]

Responded to solenbergs comments.

[stefan-webrtc, 2016-02-07 16:52:07]

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

[commit-bot: I haz the power, 2016-02-07 16:52:16]

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

[commit-bot: I haz the power, 2016-02-07 16:53:06]

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

[commit-bot: I haz the power, 2016-02-07 16:53:08]

Dry run: Try jobs failed on following builders:
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/4985)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7308)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11135)
  linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12263)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/651)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/6884)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12566)

[stefan-webrtc, 2016-02-07 17:16:02]

Rebase.

[stefan-webrtc, 2016-02-07 17:16:08]

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

[commit-bot: I haz the power, 2016-02-07 17:16:16]

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

[commit-bot: I haz the power, 2016-02-07 18:19:41]

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

[commit-bot: I haz the power, 2016-02-07 18:19:41]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-02-07 18:29:27]

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/call/congestion_co...
File webrtc/call/congestion_controller.cc (right):

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/call/congestion_co...
webrtc/call/congestion_controller.cc:146: Clock* clock,
On 2016/02/05 15:12:37, the sun wrote:
> Store clock in a field and use instead of GetRealTimeClock throughout this
file?

Good suggestion, I don't know why I didn't do that.

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_estimator.h
(right):

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_estimator.h:25:
unsigned int bitrate));
On 2016/02/05 15:12:37, the sun wrote:
> do we want this to be a uint32_t instead?

Yep, I'll make that change too.

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

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:156: remb_(remb) {
On 2016/02/05 15:12:37, the sun wrote:
> RTC_DCHECK(remb);
> wouldn't hurt to be paranoid about the other fields as well; fail early.

Done.

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

https://codereview.webrtc.org/1663413003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:134: use_config_bitrate_(true) {
On 2016/02/05 15:12:37, the sun wrote:
> Same as in receive stream, RTC_DCHECK(remb); and the others

Done.

[stefan-webrtc, 2016-02-07 18:30:45]

FYI, I plan on doing an even bigger refactoring of CongestionController in a
follow-up to this, where I get rid of "bool send_side_bwe" from
GetRemoteBitrateEstimator(), split it up into two classes, one for send-side and
one for receive-side, etc.

[the sun, 2016-02-08 10:39:23]

lgtm % comments

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h
(right):

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h:83:
virtual void RemoveStream(unsigned int ssrc) = 0;
uint32_t here as well?

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h:88:
virtual bool LatestEstimate(std::vector<unsigned int>* ssrcs,
and here

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

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:183: RTC_DCHECK(congestion_controller_);
nit: process_thread missing

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.h:51: VieRemb* remb);
For the audio streams, I've created a shared AudioState object, which is passed
down. I think it could make sense to do something similar for video. Then we
could avoid passing naked pointers like this, instead pass the state as a ref
counted object and get the various fields with accessors.

See:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
and
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

I'm not suggesting you do it in this CL, but maybe think about it?

[the sun, 2016-02-08 12:11:43]

On 2016/02/07 18:30:45, stefan-webrtc (holmer) wrote:
> FYI, I plan on doing an even bigger refactoring of CongestionController in a
> follow-up to this, where I get rid of "bool send_side_bwe" from
> GetRemoteBitrateEstimator(), split it up into two classes, one for send-side
and
> one for receive-side, etc.

Sounds good! If there's a design doc I'm happy to look at that.

[stefan-webrtc, 2016-02-08 13:11:40]

Comments addressed.

[stefan-webrtc, 2016-02-08 13:30:31]

rebase.

[stefan-webrtc, 2016-02-08 13:30:54]

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h
(right):

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h:83:
virtual void RemoveStream(unsigned int ssrc) = 0;
On 2016/02/08 10:39:23, the sun wrote:
> uint32_t here as well?

Done.

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h:88:
virtual bool LatestEstimate(std::vector<unsigned int>* ssrcs,
On 2016/02/08 10:39:23, the sun wrote:
> and here

Done.

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

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:183: RTC_DCHECK(congestion_controller_);
On 2016/02/08 10:39:23, the sun wrote:
> nit: process_thread missing

Done.

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/1663413003/diff/60001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.h:51: VieRemb* remb);
On 2016/02/08 10:39:23, the sun wrote:
> For the audio streams, I've created a shared AudioState object, which is
passed
> down. I think it could make sense to do something similar for video. Then we
> could avoid passing naked pointers like this, instead pass the state as a ref
> counted object and get the various fields with accessors.
> 
> See:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> and
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> I'm not suggesting you do it in this CL, but maybe think about it?

Possibly. pbos, what's your opinion on this?

[commit-bot: I haz the power, 2016-02-08 13:31:57]

Description was changed from

==========
Clean up of CongestionController.

Removes unused methods and moves out ViERemb to Call.
==========

to

==========
Clean up of CongestionController.

Removes unused methods and moves out ViERemb to Call.

R=pbos@webrtc.org, solenberg@webrtc.org

Committed: https://crrev.com/58c664c13dee0ed39a4d34683da5cf288455c0e5
Cr-Commit-Position: refs/heads/master@{#11527}
==========

[commit-bot: I haz the power, 2016-02-08 13:31:58]

Patchset 6 (id:??) landed as
https://crrev.com/58c664c13dee0ed39a4d34683da5cf288455c0e5
Cr-Commit-Position: refs/heads/master@{#11527}

[stefan-webrtc, 2016-02-08 13:31:59]

Description was changed from

==========
Clean up of CongestionController.

Removes unused methods and moves out ViERemb to Call.

R=pbos@webrtc.org, solenberg@webrtc.org

Committed: https://crrev.com/58c664c13dee0ed39a4d34683da5cf288455c0e5
Cr-Commit-Position: refs/heads/master@{#11527}
==========

to

==========
Clean up of CongestionController.

Removes unused methods and moves out ViERemb to Call.

R=pbos@webrtc.org, solenberg@webrtc.org

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

[stefan-webrtc, 2016-02-08 13:32:00]

Committed patchset #6 (id:100001) manually as
58c664c13dee0ed39a4d34683da5cf288455c0e5 (presubmit successful).