Split CongestionController into send- and receive-side classes.

Split CongestionController into send- and receive-side classes.

New class ReceiveSideCongestionController, extracted from CongestionController, and responsible for the
OnReceivedPacket processing.

Rest of the CongestionController moved to a new class
SendSideCongestionController.

To avoid breaking applications, CongestionController is redefined 
as a union of these two classes, with no intended change in behavior.

With one exception: CongestionController::SetBweBitrates used to call
remote_bitrate_estimator_.SetMinBitrate, but after remote_bitrate_estimator_ was moved to ReceiveSideCongestionController, 
it no longer does this.

BUG=webrtc:6847
Review-Url: https://codereview.webrtc.org/2752233002
Cr-Commit-Position: refs/heads/master@{#17321}
Committed: https://chromium.googlesource.com/external/webrtc/+/559af38a15f6590926759440002512b70d01ba58

[nisse-webrtc, 2017-03-16 13:01:06]

Description was changed from

==========
New class ReceiveSideCongestionController.

Extracted from CongestionController, and responsible for the
OnReceivedPacket processing.

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

to

==========
New class ReceiveSideCongestionController.

Extracted from CongestionController, and responsible for the
OnReceivedPacket processing.

BUG=webrtc:6847
==========

[nisse-webrtc, 2017-03-16 13:03:17]

nisse@webrtc.org changed reviewers:
+ danilchap@webrtc.org, philipel@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2017-03-16 13:03:18]

PTAL. First attempt at splitting out the receive-side processing from
CongestionController.

Will likely need some maneuvering do get this done without breaking anything.

[danilchap, 2017-03-16 13:41:52]

To avoid breaking dependent projects, is it reasonable to ask to also rename
CongestionController to SendSideCongestionController and recreate original
CongestionController with same constructors/public methods as current.
Internally it will contains ReceiveSideCongestionController and
SideSideCongestionController and route all calls to one of them (or, some case,
both)

Also, this way only congestion_controller.* files need change (even test
splitting might be postponed)

[nisse-webrtc, 2017-03-16 13:44:25]

On 2017/03/16 13:41:52, danilchap wrote:
> To avoid breaking dependent projects, is it reasonable to ask to also rename
> CongestionController to SendSideCongestionController and recreate original
> CongestionController with same constructors/public methods as current.
> Internally it will contains ReceiveSideCongestionController and
> SideSideCongestionController and route all calls to one of them (or, some
case,
> both)
> 
> Also, this way only congestion_controller.* files need change (even test
> splitting might be postponed)

Good idea!

[nisse-webrtc, 2017-03-16 14:40:50]

Description was changed from

==========
New class ReceiveSideCongestionController.

Extracted from CongestionController, and responsible for the
OnReceivedPacket processing.

BUG=webrtc:6847
==========

to

==========
Split CongestionController into send- and receive-side classes.

New class ReceiveSideCongestionController, extracted from CongestionController,
and responsible for the
OnReceivedPacket processing.

Rest of the CongestionController moved to a new class
SendSideCongestionController.

To avoid breaking applications, CongestionController is redefined 
as a union of these two classes, with no intended change in behavior.

With one exception: CongestionController::SetBweBitrates used to call
remote_bitrate_estimator_.SetMinBitrate, but after remote_bitrate_estimator_ was
moved to ReceiveSideCongestionController, 
it no longer does this.

BUG=webrtc:6847
==========

[nisse-webrtc, 2017-03-16 14:47:43]

On 2017/03/16 13:41:52, danilchap wrote:
> To avoid breaking dependent projects, is it reasonable to ask to also rename
> CongestionController to SendSideCongestionController and recreate original
> CongestionController with same constructors/public methods as current.
> Internally it will contains ReceiveSideCongestionController and
> SideSideCongestionController and route all calls to one of them (or, some
case,
> both)

Done now!

[nisse-webrtc, 2017-03-16 15:24:45]

This now appears to work without any obvious breakage.
So I hope to land this reasonably soon. All feedback appreciated.

[danilchap, 2017-03-16 18:21:42]

looks good to me

https://codereview.webrtc.org/2752233002/diff/40001/webrtc/modules/congestion...
File
webrtc/modules/congestion_controller/include/receive_side_congestion_controller.h
(right):

https://codereview.webrtc.org/2752233002/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/receive_side_congestion_controller.h:38:
// TODO(nisse): Define an interface for this?
May be later, when wiring congestion controller with rtp transport controller.
it doesn't feel these TODOs belong to this CL.

https://codereview.webrtc.org/2752233002/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/receive_side_congestion_controller.h:60:
virtual ~ReceiveSideCongestionController() {}
put destructor after contructor, before other functions

[nisse-webrtc, 2017-03-17 07:43:39]

https://codereview.webrtc.org/2752233002/diff/40001/webrtc/modules/congestion...
File
webrtc/modules/congestion_controller/include/receive_side_congestion_controller.h
(right):

https://codereview.webrtc.org/2752233002/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/receive_side_congestion_controller.h:38:
// TODO(nisse): Define an interface for this?
On 2017/03/16 18:21:42, danilchap wrote:
> May be later, when wiring congestion controller with rtp transport controller.
> it doesn't feel these TODOs belong to this CL.

Deleted this TOD. Kept the others with some smallish edits.

https://codereview.webrtc.org/2752233002/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/receive_side_congestion_controller.h:60:
virtual ~ReceiveSideCongestionController() {}
On 2017/03/16 18:21:42, danilchap wrote:
> put destructor after contructor, before other functions

Done.

[stefan-webrtc, 2017-03-17 08:03:32]

https://codereview.webrtc.org/2752233002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2752233002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:987:
receive_side_congestion_controller_.OnNetworkRouteChanged(
We only have to call this if we're using recv-side bwe. Should we put it under a
check like that, or is that just making the code more complicated?

https://codereview.webrtc.org/2752233002/diff/60001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/receive_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2752233002/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/receive_side_congestion_controller.cc:164:
int min_bitrate_bps) {
I doubt there's much value in this as I don't think anyone will be changing the
min bitrate if the network route changes. I would suggest we remove it.

[nisse-webrtc, 2017-03-17 10:49:23]

https://codereview.webrtc.org/2752233002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2752233002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:987:
receive_side_congestion_controller_.OnNetworkRouteChanged(
On 2017/03/17 08:03:32, stefan-webrtc wrote:
> We only have to call this if we're using recv-side bwe. Should we put it under
a
> check like that, or is that just making the code more complicated?

Do we have any super easy way to check if we're using recv-side bwe (for all
streams)? If not, I think it's best to keep it simple.

But even simpler is to delete the call, as you suggested in your other comment.
So I'm doing that.

https://codereview.webrtc.org/2752233002/diff/60001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/receive_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2752233002/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/receive_side_congestion_controller.cc:164:
int min_bitrate_bps) {
On 2017/03/17 08:03:32, stefan-webrtc wrote:
> I doubt there's much value in this as I don't think anyone will be changing
the
> min bitrate if the network route changes. I would suggest we remove it.

Sounds good to me. Deleted.

[nisse-webrtc, 2017-03-20 10:53:06]

nisse@webrtc.org changed reviewers:
+ elad.alon@webrtc.org

[nisse-webrtc, 2017-03-20 10:53:07]

+elad, I had to solve rebase conflict on top of 
cl https://codereview.webrtc.org/2752353003

PTAL.

[nisse-webrtc, 2017-03-20 11:35:16]

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

[commit-bot: I haz the power, 2017-03-20 11:35:18]

Dry run: 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-03-20 11:55:48]

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

[commit-bot: I haz the power, 2017-03-20 11:55:49]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-03-20 13:35:04]

Ping? I'd like to make progress on this one.

[stefan-webrtc, 2017-03-20 13:36:47]

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

https://codereview.webrtc.org/2752233002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:40: //
remote_bitrate_estimator_.SetMinBitrate. Is that needed?
Since we have removed it I think we should remove the comment too.

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

https://codereview.webrtc.org/2752233002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/receive_side_congestion_controller.cc:161:
// calls to OnBitRateChanged here?
I guess it would be good to reset the remote_bitrate_estimator on a network
route change.

remote_estimator_proxy_.OnBitrateChanged(bitrate_bps) was called to notify the
feedback sender about what the BWE is on the uplink so that we can reduce the
frequency of feedback on low bitrate links. This is a bit more complicated to do
now... I guess we'll have to notify the receive BWE from
Call::OnNetworkChanged()?

[nisse-webrtc, 2017-03-20 14:07:06]

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

https://codereview.webrtc.org/2752233002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:40: //
remote_bitrate_estimator_.SetMinBitrate. Is that needed?
On 2017/03/20 13:36:46, stefan-webrtc wrote:
> Since we have removed it I think we should remove the comment too.

Done.

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

https://codereview.webrtc.org/2752233002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/receive_side_congestion_controller.cc:161:
// calls to OnBitRateChanged here?
On 2017/03/20 13:36:46, stefan-webrtc wrote:
> I guess it would be good to reset the remote_bitrate_estimator on a network
> route change.

Can you be more concrete on what you suggest?

> remote_estimator_proxy_.OnBitrateChanged(bitrate_bps) was called to notify the
> feedback sender about what the BWE is on the uplink so that we can reduce the
> frequency of feedback on low bitrate links. This is a bit more complicated to
do
> now... I guess we'll have to notify the receive BWE from
> Call::OnNetworkChanged()?

This part done.

[elad.alon_webrtc.org, 2017-03-20 16:23:46]

On 2017/03/20 10:53:07, nisse-webrtc wrote:
> +elad, I had to solve rebase conflict on top of 
> cl https://codereview.webrtc.org/2752353003
> 
> PTAL.

I see no problems from my side.

[philipel, 2017-03-20 16:48:19]

lgtm, just some minor comments about style.

https://codereview.webrtc.org/2752233002/diff/120001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.h (right):

https://codereview.webrtc.org/2752233002/diff/120001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.h:80: SendSideCongestionController* const
congestion_controller_;
WDYT about renaming this to |send_side_cc_|, or to
|send_side_congestion_controller_| which is used in CongestionController?

https://codereview.webrtc.org/2752233002/diff/120001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2752233002/diff/120001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:414: SendSideCongestionController* const
congestion_controller_;
Same here, WDYT about renaming this to |send_side_cc_|, or to
|send_side_congestion_controller_| which is used in CongestionController?

[nisse-webrtc, 2017-03-21 08:50:14]

On 2017/03/20 16:48:19, philipel wrote:
> lgtm, just some minor comments about style.
> 
>
https://codereview.webrtc.org/2752233002/diff/120001/webrtc/audio/audio_send_...
> File webrtc/audio/audio_send_stream.h (right):
> 
>
https://codereview.webrtc.org/2752233002/diff/120001/webrtc/audio/audio_send_...
> webrtc/audio/audio_send_stream.h:80: SendSideCongestionController* const
> congestion_controller_;
> WDYT about renaming this to |send_side_cc_|, or to
> |send_side_congestion_controller_| which is used in CongestionController?
> 
>
https://codereview.webrtc.org/2752233002/diff/120001/webrtc/video/video_send_...
> File webrtc/video/video_send_stream.cc (right):
> 
>
https://codereview.webrtc.org/2752233002/diff/120001/webrtc/video/video_send_...
> webrtc/video/video_send_stream.cc:414: SendSideCongestionController* const
> congestion_controller_;
> Same here, WDYT about renaming this to |send_side_cc_|, or to
> |send_side_congestion_controller_| which is used in CongestionController?

I've changed to use |send_side_cc_| and |receive_side_cc| as variable names
everywhere.

[stefan-webrtc, 2017-03-21 12:32:18]

lgtm

[nisse-webrtc, 2017-03-21 13:09:05]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-03-21 13:09:05]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2752233002/#ps140001 (title:
"Use variable names receive_side_cc and send_side_cc.")

[commit-bot: I haz the power, 2017-03-21 13:09: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-03-21 13:41:13]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490101745230690,
"parent_rev": "6a64bd96bf8cdf795487cd4433415b7f85061927", "commit_rev":
"559af38a15f6590926759440002512b70d01ba58"}

[commit-bot: I haz the power, 2017-03-21 13:41:17]

Description was changed from

==========
Split CongestionController into send- and receive-side classes.

New class ReceiveSideCongestionController, extracted from CongestionController,
and responsible for the
OnReceivedPacket processing.

Rest of the CongestionController moved to a new class
SendSideCongestionController.

To avoid breaking applications, CongestionController is redefined 
as a union of these two classes, with no intended change in behavior.

With one exception: CongestionController::SetBweBitrates used to call
remote_bitrate_estimator_.SetMinBitrate, but after remote_bitrate_estimator_ was
moved to ReceiveSideCongestionController, 
it no longer does this.

BUG=webrtc:6847
==========

to

==========
Split CongestionController into send- and receive-side classes.

New class ReceiveSideCongestionController, extracted from CongestionController,
and responsible for the
OnReceivedPacket processing.

Rest of the CongestionController moved to a new class
SendSideCongestionController.

To avoid breaking applications, CongestionController is redefined 
as a union of these two classes, with no intended change in behavior.

With one exception: CongestionController::SetBweBitrates used to call
remote_bitrate_estimator_.SetMinBitrate, but after remote_bitrate_estimator_ was
moved to ReceiveSideCongestionController, 
it no longer does this.

BUG=webrtc:6847

Review-Url: https://codereview.webrtc.org/2752233002
Cr-Commit-Position: refs/heads/master@{#17321}
Committed:
https://chromium.googlesource.com/external/webrtc/+/559af38a15f65909267594400...
==========

[commit-bot: I haz the power, 2017-03-21 13:41:20]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/559af38a15f65909267594400...