Move PacedSender ownership to RtpTransportControllerSend.

Move PacedSender ownership to RtpTransportControllerSend.

BUG=webrtc:8089
R=nisse@webrtc.org, terelius@webrtc.org

Review-Url: https://codereview.webrtc.org/3000773002 .
Cr-Commit-Position: refs/heads/master@{#19451}
Committed: https://chromium.googlesource.com/external/webrtc/+/5c8942aee1f2003bd43c8e4427fb74e6f1015fc1

[stefan-webrtc, 2017-08-11 13:53:35]

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

[stefan-webrtc, 2017-08-11 13:53:38]

https://codereview.webrtc.org/3000773002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/3000773002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:105:
rtc::MakeUnique<PacedSender>(clock, packet_router, event_log)),
This is now a copy of the constructor below, besides this line. Will be removed
soon.

[stefan-webrtc, 2017-08-11 13:53:44]

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

[commit-bot: I haz the power, 2017-08-11 13:53:51]

Dry run: CQ is trying da patch.

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

[stefan-webrtc, 2017-08-11 13:54:20]

Description was changed from

==========
Moved PacedSender ownership to RtpTransportControllerSend

BUG=webrtc:8089
==========

to

==========
Move PacedSender ownership to RtpTransportControllerSend.

BUG=webrtc:8089
==========

[commit-bot: I haz the power, 2017-08-11 13:57:22]

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

[commit-bot: I haz the power, 2017-08-11 13:57:23]

Dry run: Try jobs failed on following builders:
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[stefan-webrtc, 2017-08-11 14:04:21]

Fix dependency issue.

[stefan-webrtc, 2017-08-11 16:51:04]

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

[commit-bot: I haz the power, 2017-08-11 16:51:13]

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-08-11 16:53:43]

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

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

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  linux32_arm_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux32_arm_rel/builds/...)
  linux_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/14506)

[stefan-webrtc, 2017-08-11 16:54:06]

The CQ bit was unchecked by stefan@webrtc.org

[stefan-webrtc, 2017-08-11 17:34:49]

.

[stefan-webrtc, 2017-08-11 17:38:52]

The CQ bit was unchecked by stefan@webrtc.org

[stefan-webrtc, 2017-08-11 17:38:52]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2017-08-11 17:39:03]

CQ is trying da patch.

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

[stefan-webrtc, 2017-08-11 17:39:04]

The CQ bit was unchecked by stefan@webrtc.org

[commit-bot: I haz the power, 2017-08-11 17:39:05]

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

[commit-bot: I haz the power, 2017-08-11 17:39:06]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-webrtc-committers".
Note that this has nothing to do with OWNERS files.

[stefan-webrtc, 2017-08-11 17:41:08]

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

[commit-bot: I haz the power, 2017-08-11 17:41:11]

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-08-11 17:50:43]

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

[commit-bot: I haz the power, 2017-08-11 17:50:44]

Dry run: Try jobs failed on following builders:
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/28058)

[stefan-webrtc, 2017-08-14 13:26:02]

Fix test.

[stefan-webrtc, 2017-08-14 13:26:09]

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

[commit-bot: I haz the power, 2017-08-14 13:26:13]

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-08-14 13:30:44]

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

[commit-bot: I haz the power, 2017-08-14 13:30:45]

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

[stefan-webrtc, 2017-08-17 09:06:56]

ping

[stefan-webrtc, 2017-08-17 09:07:37]

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

[stefan-webrtc, 2017-08-17 09:07:38]

Nisse may also care.

[terelius, 2017-08-17 13:49:15]

looks good, but the unit tests don't pass

[nisse-webrtc, 2017-08-18 06:19:18]

I like this. A couple of questions.

https://codereview.webrtc.org/3000773002/diff/80001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_send.cc (right):

https://codereview.webrtc.org/3000773002/diff/80001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_send.cc:25: PacedSender*
RtpTransportControllerSend::pacer() {
The packet_sender() method below returns the same object, but using an interface
type. I think this deserves a comment, with a brief explanation why this method
is needed, and whether or not we'll aim to delete it. If it's intended to stay,
I'd prefer to delete the other packet_sender method which becomes redundant.

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

https://codereview.webrtc.org/3000773002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:259:
void SendSideCongestionController::SetAllocatedSendBitrateLimits(
From where is this called? Would it make sense to move this method (and others
involving pacer details) to the transport controller too?

[nisse-webrtc, 2017-08-18 07:14:33]

https://codereview.webrtc.org/3000773002/diff/80001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_send.cc (right):

https://codereview.webrtc.org/3000773002/diff/80001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_send.cc:25: PacedSender*
RtpTransportControllerSend::pacer() {
On 2017/08/18 06:19:18, nisse-webrtc wrote:
> The packet_sender() method below returns the same object, but using an
interface
> type. I think this deserves a comment, with a brief explanation why this
method
> is needed, and whether or not we'll aim to delete it. If it's intended to
stay,
> I'd prefer to delete the other packet_sender method which becomes redundant.

And yet another option for the return type is RtpPacketSender, which I attempted
to use in cl https://codereview.webrtc.org/2580843002/

[stefan-webrtc, 2017-08-21 14:42:41]

rebase

[stefan-webrtc, 2017-08-22 07:37:36]

Comments addressed.

[stefan-webrtc, 2017-08-22 07:37:47]

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

[commit-bot: I haz the power, 2017-08-22 07:37:50]

Dry run: CQ is trying da patch.

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

[stefan-webrtc, 2017-08-22 07:37:53]

https://codereview.webrtc.org/3000773002/diff/80001/webrtc/call/rtp_transport...
File webrtc/call/rtp_transport_controller_send.cc (right):

https://codereview.webrtc.org/3000773002/diff/80001/webrtc/call/rtp_transport...
webrtc/call/rtp_transport_controller_send.cc:25: PacedSender*
RtpTransportControllerSend::pacer() {
On 2017/08/18 07:14:33, nisse-webrtc wrote:
> On 2017/08/18 06:19:18, nisse-webrtc wrote:
> > The packet_sender() method below returns the same object, but using an
> interface
> > type. I think this deserves a comment, with a brief explanation why this
> method
> > is needed, and whether or not we'll aim to delete it. If it's intended to
> stay,
> > I'd prefer to delete the other packet_sender method which becomes redundant.
> 
> And yet another option for the return type is RtpPacketSender, which I
attempted
> to use in cl https://codereview.webrtc.org/2580843002/

For now we need to return a PacedSender as we have to be able to register it to
the process thread. For that we could return a Module, but we're currently also
calling some methods on the PacedSender from the video streams. I think both
should be possible to get rid of later by adding a few methods to the
RtpTransportControllerSend instead of exposing the pacer directly.

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

https://codereview.webrtc.org/3000773002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:259:
void SendSideCongestionController::SetAllocatedSendBitrateLimits(
On 2017/08/18 06:19:18, nisse-webrtc wrote:
> From where is this called? Would it make sense to move this method (and others
> involving pacer details) to the transport controller too?

I've moved this now too. There are more things to move though
(GetPacerQueueingDelayMs for instance), and my plan is to do that separately to
keep the CL size smaller.

[commit-bot: I haz the power, 2017-08-22 07:48:17]

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

[commit-bot: I haz the power, 2017-08-22 07:48:18]

Dry run: Try jobs failed on following builders:
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/28409)

[nisse-webrtc, 2017-08-22 08:06:58]

lgtm

[stefan-webrtc, 2017-08-22 08:24:18]

Fix test failure.

[stefan-webrtc, 2017-08-22 08:24:26]

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

[commit-bot: I haz the power, 2017-08-22 08:24:37]

Dry run: CQ is trying da patch.

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

[stefan-webrtc, 2017-08-22 08:26:01]

terelius, any more comments from you? I hope the tests will be passing now.

[commit-bot: I haz the power, 2017-08-22 08:42:28]

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

[commit-bot: I haz the power, 2017-08-22 08:42:29]

Dry run: 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/27337)

[terelius, 2017-08-22 11:25:20]

lgtm

[stefan-webrtc, 2017-08-22 11:51:44]

Fix test bug.

[stefan-webrtc, 2017-08-22 11:53:48]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2017-08-22 11:53:49]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3000773002/#ps160001 (title:
"Fix test bug.")

[commit-bot: I haz the power, 2017-08-22 11:53:58]

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-08-22 12:18:49]

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

[commit-bot: I haz the power, 2017-08-22 12:18:51]

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/27348)

[stefan-webrtc, 2017-08-22 13:13:49]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2017-08-22 13:13:56]

CQ is trying da patch.

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

[stefan-webrtc, 2017-08-22 14:16:56]

Description was changed from

==========
Move PacedSender ownership to RtpTransportControllerSend.

BUG=webrtc:8089
==========

to

==========
Move PacedSender ownership to RtpTransportControllerSend.

BUG=webrtc:8089
R=nisse@webrtc.org, terelius@webrtc.org

Review-Url: https://codereview.webrtc.org/3000773002 .
Cr-Commit-Position: refs/heads/master@{#19451}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5c8942aee1f2003bd43c8e442...
==========

[stefan-webrtc, 2017-08-22 14:16:58]

Committed patchset #9 (id:160001) manually as
5c8942aee1f2003bd43c8e4427fb74e6f1015fc1 (presubmit successful).

[kjellander_webrtc, 2017-08-23 06:04:55]

kjellander@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kjellander_webrtc, 2017-08-23 06:04:56]

This was submitted as holmer@chromium.org due to manual commit, halting our
import pipeline. Stefan: please setup git config stefan@webrtc.org in your
WebRTC checkout or always use CQ (NOTRY=True to bypass trybots)

[stefan-webrtc, 2017-08-23 06:57:30]

On 2017/08/23 06:04:56, kjellander_webrtc wrote:
> This was submitted as mailto:holmer@chromium.org due to manual commit, halting
our
> import pipeline. Stefan: please setup git config mailto:stefan@webrtc.org in
your
> WebRTC checkout or always use CQ (NOTRY=True to bypass trybots)

ah... sorry, must have messed this up when I tried to get my chromium account
working a few months back.