Define RtpTransportControllerSendInterface.

Define RtpTransportControllerSendInterface.

Implementation owned by call, and passed to VideoSendStream and
AudioSendStream.

BUG=webrtc:6847, webrtc:7135
Review-Url: https://codereview.webrtc.org/2685673003
Cr-Commit-Position: refs/heads/master@{#17389}
Committed: https://chromium.googlesource.com/external/webrtc/+/b8f9a324590b0c412c414ccaddfebd8aae12471f

[nisse-webrtc, 2017-02-08 14:12:53]

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

[nisse-webrtc, 2017-02-08 14:12:53]

This is a first step towards RtpTransportController interfaces.

Comments appreciated.

About naming, we really need something a bit shorter than
RtpTransportControllerSenderInterface. 
And I'm also not sure where I should put the header files. api/rtp/ doesn't feel
right until interfaces are close to finished. For now I put them under call/
(which is somewhat logical, since call owns the only implementation), but that
had the drawback that I have to add a gn dependency in voice_engine.

[Taylor Brandstetter, 2017-02-08 23:35:51]

On 2017/02/08 14:12:53, nisse-webrtc wrote:
> This is a first step towards RtpTransportController interfaces.
> 
> Comments appreciated.
> 
> About naming, we really need something a bit shorter than
> RtpTransportControllerSenderInterface. 
> And I'm also not sure where I should put the header files. api/rtp/ doesn't
feel
> right until interfaces are close to finished. For now I put them under call/
> (which is somewhat logical, since call owns the only implementation), but that
> had the drawback that I have to add a gn dependency in voice_engine.

The approach of putting the interfaces in call/ until they're stable sounds
good.

Why is this not simply called RtpTransportControllerInterface, though? I guess
I'm not clear about whether this is meant to represent "RtpTransportController"
or "RtpTransportSender" from the design doc.

[nisse-webrtc, 2017-02-09 07:31:24]

On 2017/02/08 23:35:51, Taylor Brandstetter wrote:
> Why is this not simply called RtpTransportControllerInterface, though? I guess
> I'm not clear about whether this is meant to represent
"RtpTransportController"
> or "RtpTransportSender" from the design doc.

My current thinking is that we should split the functionality of
RtpTransportController into separate interfaces for send and and receive side.
The RtpTransportControllerSenderInterface represents the features of
RtpTransportController which are relevant to senders, and that's what is passed
to the constructors of *SendStream.

There should be a corresponding RtpTransportControllerReceiverInterface, with
the features relevant to receive streams, which would be passed to
*ReceiveStream constructors. Neither correspond to RtpTransportSender.

For Call, I think we'll have a single RtpTransportController implementing both
interfaces. But if some other application is send-only, it can do that by
implementing the RtpTransportControllerSenderInterface only, and pass its
implementation when creating send streams.

A small improvement to naming could be to drop "er" and call it
RtpTransportControllerSendInterface. But I'd prefer something a bit shorter.

[nisse-webrtc, 2017-02-09 07:31:55]

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

[nisse-webrtc, 2017-02-09 07:31:55]

+danil

[Taylor Brandstetter, 2017-02-09 08:49:44]

On 2017/02/09 07:31:24, nisse-webrtc wrote:
> On 2017/02/08 23:35:51, Taylor Brandstetter wrote:
> > Why is this not simply called RtpTransportControllerInterface, though? I
guess
> > I'm not clear about whether this is meant to represent
> "RtpTransportController"
> > or "RtpTransportSender" from the design doc.
> 
> My current thinking is that we should split the functionality of
> RtpTransportController into separate interfaces for send and and receive side.
> The RtpTransportControllerSenderInterface represents the features of
> RtpTransportController which are relevant to senders, and that's what is
passed
> to the constructors of *SendStream.
> 
> There should be a corresponding RtpTransportControllerReceiverInterface, with
> the features relevant to receive streams, which would be passed to
> *ReceiveStream constructors. Neither correspond to RtpTransportSender.
> 
> For Call, I think we'll have a single RtpTransportController implementing both
> interfaces. But if some other application is send-only, it can do that by
> implementing the RtpTransportControllerSenderInterface only, and pass its
> implementation when creating send streams.
> 
> A small improvement to naming could be to drop "er" and call it
> RtpTransportControllerSendInterface. But I'd prefer something a bit shorter.

Ah, I see. Though, unless there plans for an application to implement a
send-only or receive-only controller, I think it would be simpler to avoid this
split for now.

[stefan-webrtc, 2017-02-09 08:55:37]

On 2017/02/09 08:49:44, Taylor Brandstetter wrote:
> On 2017/02/09 07:31:24, nisse-webrtc wrote:
> > On 2017/02/08 23:35:51, Taylor Brandstetter wrote:
> > > Why is this not simply called RtpTransportControllerInterface, though? I
> guess
> > > I'm not clear about whether this is meant to represent
> > "RtpTransportController"
> > > or "RtpTransportSender" from the design doc.
> > 
> > My current thinking is that we should split the functionality of
> > RtpTransportController into separate interfaces for send and and receive
side.
> > The RtpTransportControllerSenderInterface represents the features of
> > RtpTransportController which are relevant to senders, and that's what is
> passed
> > to the constructors of *SendStream.
> > 
> > There should be a corresponding RtpTransportControllerReceiverInterface,
with
> > the features relevant to receive streams, which would be passed to
> > *ReceiveStream constructors. Neither correspond to RtpTransportSender.
> > 
> > For Call, I think we'll have a single RtpTransportController implementing
both
> > interfaces. But if some other application is send-only, it can do that by
> > implementing the RtpTransportControllerSenderInterface only, and pass its
> > implementation when creating send streams.
> > 
> > A small improvement to naming could be to drop "er" and call it
> > RtpTransportControllerSendInterface. But I'd prefer something a bit shorter.
> 
> Ah, I see. Though, unless there plans for an application to implement a
> send-only or receive-only controller, I think it would be simpler to avoid
this
> split for now.

I think we should at least try to split them in the implementation of the
RtpTransportController to avoid weird sender/receiver dependencies. The
interface may not have to be split at this point.

[danilchap, 2017-02-09 09:04:04]

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

https://codereview.webrtc.org/2685673003/diff/1/webrtc/call/call.cc#newcode92
webrtc/call/call.cc:92: namespace internal {
just merge this with unnamed namespace.
namespace internal is helpful for header files where unnamed namespace is
forbidden.

https://codereview.webrtc.org/2685673003/diff/1/webrtc/call/call.cc#newcode102
webrtc/call/call.cc:102: static std::unique_ptr<RtpTransportController> Create(
why have an extra Create method?
This class already in .cc file so it can be constructed from this file only
anyway.

https://codereview.webrtc.org/2685673003/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller.h:14: #include
"webrtc/modules/congestion_controller/include/congestion_controller.h"
why include congestion controller and forward declare it?

https://codereview.webrtc.org/2685673003/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller.h:31: // of all RTP-related objects here,
and add methods to create per-ssrc
probably per-(media)-stream instead of per-ssrc:
Do you want separate object for rtx or not?
May be you want separate object for flexfec (since it can protect multipe
media-streams and thus doesn't belong to any)
In quic there will be no ssrc (most likely), but we might want to use same
interface.

[nisse-webrtc, 2017-02-09 12:56:43]

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

https://codereview.webrtc.org/2685673003/diff/1/webrtc/call/call.cc#newcode102
webrtc/call/call.cc:102: static std::unique_ptr<RtpTransportController> Create(
On 2017/02/09 09:04:04, danilchap wrote:
> why have an extra Create method?

Only to conveniently get a unique_ptr. 

> This class already in .cc file so it can be constructed from this file only
> anyway.

Ok, deleting the method.

https://codereview.webrtc.org/2685673003/diff/1/webrtc/call/rtp_transport_con...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller.h:14: #include
"webrtc/modules/congestion_controller/include/congestion_controller.h"
On 2017/02/09 09:04:04, danilchap wrote:
> why include congestion controller and forward declare it?

Good catch. Dropping forward declaration (include is needed for
voice_engine/channel.cc).

https://codereview.webrtc.org/2685673003/diff/1/webrtc/call/rtp_transport_con...
webrtc/call/rtp_transport_controller.h:31: // of all RTP-related objects here,
and add methods to create per-ssrc
On 2017/02/09 09:04:04, danilchap wrote:
> probably per-(media)-stream instead of per-ssrc:
> Do you want separate object for rtx or not?
> May be you want separate object for flexfec (since it can protect multipe
> media-streams and thus doesn't belong to any)
> In quic there will be no ssrc (most likely), but we might want to use same
> interface.

The plan is that VideoSendStream should create one RtpTransportSender per ssrc
it uses.

[nisse-webrtc, 2017-02-09 16:27:41]

On 2017/02/09 08:55:37, stefan-webrtc wrote:
> I think we should at least try to split them in the implementation of the
> RtpTransportController to avoid weird sender/receiver dependencies. The
> interface may not have to be split at this point.

We'll see what dependencies there are between send and receive side. If it 
only boils down to the receive side needing a way to transmit rtcp packets
with feedback, we should definitely split the implementation.

As for the interfaces, I think it makes it clearer at this point that
the RtpTransportController interface which gives access to the congestion
controller and related object isn't passed at all to the receive side.

[nisse-webrtc, 2017-02-14 08:38:42]

What do you think is needed to make progress?

To me, the cl seems landable as is (even if arguably premature).

[nisse-webrtc, 2017-02-14 08:39:14]

Description was changed from

==========
Define RtpTransportControllerSenderInterface.

Implementation owned by call, and passed to VideoSendStream and
AudioSendStream.

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

to

==========
Define RtpTransportControllerSendInterface.

Implementation owned by call, and passed to VideoSendStream and
AudioSendStream.

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

[stefan-webrtc, 2017-02-17 11:59:28]

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:90: class RtpTransportController : public
RtpTransportControllerSendInterface {
Maybe create a new file for this already?

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:107: // argument, and setting it later seems no-trivial.
Doesn't this just mean that we're creating the RtpTransportController in the
wrong place? If we create Call and/or the BandwidthAllocator before the
RtpTransportController this wouldn't be a problem, right? Seems to me that we
might want to enforce that call order.

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:778: ->GetRemoteBitrateEstimator(UseSendSideBwe(config))
What would it take to get rid of GetRemoteBitrateEstimator? Would it be enough
to expose methods on CongestionController, such as, RemoveStream(uint32_t id)
and LatestEstimate() (which probably should be called
GetEstimatedRecvBandwidth() or something)?

I think that would be a nice cleanup.

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:23: // interfaces for send and receive
sice, even if they are implemented
since

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:24: // by the same class. This is on
ongoing refactoring project. At some
-on

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:49: virtual VieRemb* remb() = 0;
Do you think it would be possible to use PacketRouter for sending remb instead
of VieRemb (basically merge them)?

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:50: virtual PacketRouter* packet_router()
= 0;
PacketRouter will not have to be exposed any longer when RtpRtcp modules are
owned by this class, right? Maybe comment on that.

[nisse-webrtc, 2017-02-17 12:35:15]

Thanks for review. I'll prepare a new patchset next week.

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:90: class RtpTransportController : public
RtpTransportControllerSendInterface {
On 2017/02/17 11:59:28, stefan-webrtc wrote:
> Maybe create a new file for this already?

Could do that. Then I'd need to declare it (or at least some way to create it)
in the header file.

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:107: // argument, and setting it later seems no-trivial.
On 2017/02/17 11:59:28, stefan-webrtc wrote:
> Doesn't this just mean that we're creating the RtpTransportController in the
> wrong place? If we create Call and/or the BandwidthAllocator before the
> RtpTransportController this wouldn't be a problem, right? Seems to me that we
> might want to enforce that call order.

That would make sense for now, but longer term, I think one should be able to
create the transport and pass it to the Call constructor. And then one would
need to register the CongestionController's observer later, in some way.

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:778: ->GetRemoteBitrateEstimator(UseSendSideBwe(config))
On 2017/02/17 11:59:28, stefan-webrtc wrote:
> What would it take to get rid of GetRemoteBitrateEstimator? Would it be enough
> to expose methods on CongestionController, such as, RemoveStream(uint32_t id)
> and LatestEstimate() (which probably should be called
> GetEstimatedRecvBandwidth() or something)?
> 
> I think that would be a nice cleanup.

I agree this could use some cleanup, but that's for another cl.

It's also not clear to me which methods should be advertised by
CongestionController, and which should be attached to the
RtpTransportController.

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/100001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:50: virtual PacketRouter* packet_router()
= 0;
On 2017/02/17 11:59:28, stefan-webrtc wrote:
> PacketRouter will not have to be exposed any longer when RtpRtcp modules are
> owned by this class, right? Maybe comment on that.

I don't think any of the current accessor methods should be kept.

[stefan-webrtc, 2017-03-14 15:58:21]

I would very much like to get this into landable state. To me it looks good, but
I'd like for all of us to agree on that this is a step in the direction we've
been talking about before we land it.

Taylor, Danil, any comments?

[danilchap, 2017-03-14 17:59:15]

Probably transport controller should contain more objects, e.g. rtc event log
and call_stats, but that can be added later.
This CL lgtm as a step in the right direction.

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:33: // objects which would then be passed
to VideoSendStream.
May be add a comment that direct accessors (remb(), packet_router(), etc.)
probably should be eventually removed when interface will be filled with
stream factories like
std::unique_ptr<PerSsrcObject> CreateStream(uint32_t ssrc, ...);
helper functions, e.g.
void GetStats(Stats* out) const;
void SetApplicationLimitedSendBitrate(int max_bitrate_bps);
and input function(s), i.e.
void ProcessRtcpPacket(rtc::Buffer raw_packet);

[Taylor Brandstetter, 2017-03-14 23:08:35]

This looks good from my perspective. FYI, I added this interface at the API
level, implemented as an adapter on top of BaseChannel:
https://cs.chromium.org/chromium/src/third_party/webrtc/api/ortc/rtptransport...

So as long as that interface and this one continue representing the same thing
conceptually, I think they should be able to converge once the
BaseChannel/MediaChannel code is cleaned up.

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:36: // webrtc::Transport. Currently,
webrtc::Transport is implemented by
"webrtc::Transport(s)", since there may be multiple? I'm still assuming that
when audio/video use separate ports, there are two webrtc::Transports and one
RtpTransportController.

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:39: // objects, even in the common case
where they are bundeled over the
"bundled"

[nisse-webrtc, 2017-03-15 08:06:23]

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:33: // objects which would then be passed
to VideoSendStream.
On 2017/03/14 17:59:14, danilchap wrote:
> May be add a comment that direct accessors (remb(), packet_router(), etc.)
> probably should be eventually removed when interface will be filled with
> stream factories like
> std::unique_ptr<PerSsrcObject> CreateStream(uint32_t ssrc, ...);
> helper functions, e.g.
> void GetStats(Stats* out) const;
> void SetApplicationLimitedSendBitrate(int max_bitrate_bps);
> and input function(s), i.e.
> void ProcessRtcpPacket(rtc::Buffer raw_packet);

Done.

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:36: // webrtc::Transport. Currently,
webrtc::Transport is implemented by
On 2017/03/14 23:08:35, Taylor Brandstetter wrote:
> "webrtc::Transport(s)", since there may be multiple? I'm still assuming that
> when audio/video use separate ports, there are two webrtc::Transports and one
> RtpTransportController.

Updated the comment. On the underlying question, I'm not sure I agree. My
thinking has been that Call::CreateVideoSendStream would pass a
RtpTransportControllerSendInterface to the constructor of VideoSendStream. And
VideoSendStream would then ask it it to create two per-ssrc objects, one for
media and one for RTX. 

But if RtpTransportControllerSendInterface represents multiple transports, how
would VideoSendStream know which transport to use? It shouldn't need to know
anything about that.

Maybe one could move creation of the per-ssrc objects to Call (it makes some
sense that Call is aware about transports and bundled vs unbundled), but the
drawback is that Call then would need to knwo about RTX. Or have a sepatate
Call::CreateVideoRtxStream. So it's a bit tricky to get right.

Thinking aloud, one possible solution might be to let the RtpTransportController
object handle multiple transports, but expose one
RtpTransportControllerSendInterface per transport.

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:39: // objects, even in the common case
where they are bundeled over the
On 2017/03/14 23:08:35, Taylor Brandstetter wrote:
> "bundled"

Done.

[Taylor Brandstetter, 2017-03-15 21:39:08]

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:36: // webrtc::Transport. Currently,
webrtc::Transport is implemented by
On 2017/03/15 08:06:23, nisse-webrtc wrote:
> On 2017/03/14 23:08:35, Taylor Brandstetter wrote:
> > "webrtc::Transport(s)", since there may be multiple? I'm still assuming that
> > when audio/video use separate ports, there are two webrtc::Transports and
one
> > RtpTransportController.
> 
> Updated the comment. On the underlying question, I'm not sure I agree. My
> thinking has been that Call::CreateVideoSendStream would pass a
> RtpTransportControllerSendInterface to the constructor of VideoSendStream. And
> VideoSendStream would then ask it it to create two per-ssrc objects, one for
> media and one for RTX. 
> 
> But if RtpTransportControllerSendInterface represents multiple transports, how
> would VideoSendStream know which transport to use? It shouldn't need to know
> anything about that.
> 
> Maybe one could move creation of the per-ssrc objects to Call (it makes some
> sense that Call is aware about transports and bundled vs unbundled), but the
> drawback is that Call then would need to knwo about RTX. Or have a sepatate
> Call::CreateVideoRtxStream. So it's a bit tricky to get right.
> 
> Thinking aloud, one possible solution might be to let the
RtpTransportController
> object handle multiple transports, but expose one
> RtpTransportControllerSendInterface per transport.

I think we may be using the same name (RtpTransportController) to refer to
different things.

In the model I had in mind, "RtpTransport" is something that encapsulates the
underlying packet transport(s) (one if RTCP muxing is used, two otherwise), and
represents an independent RTP session/SSRC namespace.

"RtpTransportController" is something that comes into play when media is not
bundled, meaning there are separate "RtpTransport"s for audio and video, but
attached to the same RtpTransportController in order to share a congestion
controller, do A/V sync, etc.

It seems like what you're calling "RtpTransportController", I'm calling
"RtpTransport". I'm open to changing names or changing the model I just
described, but we should clear up this distinction before it gets too late.

[danilchap, 2017-03-16 11:38:12]

On 2017/03/15 21:39:08, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
> File webrtc/call/rtp_transport_controller.h (right):
> 
>
https://codereview.webrtc.org/2685673003/diff/160001/webrtc/call/rtp_transpor...
> webrtc/call/rtp_transport_controller.h:36: // webrtc::Transport. Currently,
> webrtc::Transport is implemented by
> On 2017/03/15 08:06:23, nisse-webrtc wrote:
> > On 2017/03/14 23:08:35, Taylor Brandstetter wrote:
> > > "webrtc::Transport(s)", since there may be multiple? I'm still assuming
that
> > > when audio/video use separate ports, there are two webrtc::Transports and
> one
> > > RtpTransportController.
> > 
> > Updated the comment. On the underlying question, I'm not sure I agree. My
> > thinking has been that Call::CreateVideoSendStream would pass a
> > RtpTransportControllerSendInterface to the constructor of VideoSendStream.
And
> > VideoSendStream would then ask it it to create two per-ssrc objects, one for
> > media and one for RTX. 
> > 
> > But if RtpTransportControllerSendInterface represents multiple transports,
how
> > would VideoSendStream know which transport to use? It shouldn't need to know
> > anything about that.
> > 
> > Maybe one could move creation of the per-ssrc objects to Call (it makes some
> > sense that Call is aware about transports and bundled vs unbundled), but the
> > drawback is that Call then would need to knwo about RTX. Or have a sepatate
> > Call::CreateVideoRtxStream. So it's a bit tricky to get right.
> > 
> > Thinking aloud, one possible solution might be to let the
> RtpTransportController
> > object handle multiple transports, but expose one
> > RtpTransportControllerSendInterface per transport.
> 
> I think we may be using the same name (RtpTransportController) to refer to
> different things.
> 
> In the model I had in mind, "RtpTransport" is something that encapsulates the
> underlying packet transport(s) (one if RTCP muxing is used, two otherwise),
and
> represents an independent RTP session/SSRC namespace.
> 
> "RtpTransportController" is something that comes into play when media is not
> bundled, meaning there are separate "RtpTransport"s for audio and video, but
> attached to the same RtpTransportController in order to share a congestion
> controller, do A/V sync, etc.
> 
> It seems like what you're calling "RtpTransportController", I'm calling
> "RtpTransport". I'm open to changing names or changing the model I just
> described, but we should clear up this distinction before it gets too late.

would be nice to agree what we mean by RtpTransportController (and other terms).
I see it as a cap class for everything needed for single transport(+direction):
transport itself, congestion controller, pacer, bandwidth estimator, demuxer,
list of streams using this particular transport, transport related stats, etc.

[danilchap, 2017-03-16 11:38:59]

https://codereview.webrtc.org/2685673003/diff/180001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/180001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:11: #ifndef
WEBRTC_CALL_RTP_TRANSPORT_CONTROLLER_H_
may be rename this file to rtp_transport_controller_sender

[nisse-webrtc, 2017-03-23 09:16:17]

https://codereview.webrtc.org/2685673003/diff/180001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/180001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:11: #ifndef
WEBRTC_CALL_RTP_TRANSPORT_CONTROLLER_H_
On 2017/03/16 11:38:58, danilchap wrote:
> may be rename this file to rtp_transport_controller_sender

I'll rename it to rtp_transport_controller_send.h, analogous to
rtp_transport_controller_receive.h, introduced in cl 
https://codereview.webrtc.org/2709723003/

[nisse-webrtc, 2017-03-23 09:38:36]

https://codereview.webrtc.org/2685673003/diff/180001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller.h (right):

https://codereview.webrtc.org/2685673003/diff/180001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller.h:11: #ifndef
WEBRTC_CALL_RTP_TRANSPORT_CONTROLLER_H_
On 2017/03/23 09:16:17, nisse-webrtc wrote:
> On 2017/03/16 11:38:58, danilchap wrote:
> > may be rename this file to rtp_transport_controller_sender
> 
> I'll rename it to rtp_transport_controller_send.h, analogous to
> rtp_transport_controller_receive.h, introduced in cl 
> https://codereview.webrtc.org/2709723003/

Done.

[stefan-webrtc, 2017-03-27 11:14:11]

lgtm % nits

https://codereview.webrtc.org/2685673003/diff/260001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2685673003/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:94: RtpTransportControllerSend(Clock* clock,
webrtc::RtcEventLog* event_log);
Empty line below

https://codereview.webrtc.org/2685673003/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:111: // argument, and setting it later seems no-trivial.
nit: non-trivial?

I think we later will want to be able to pass the congestion controller in, so
maybe it doesn't even have to be owned by the transport controller later on.

[nisse-webrtc, 2017-03-27 11:41:22]

Attempting to land this now.

https://codereview.webrtc.org/2685673003/diff/260001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2685673003/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:94: RtpTransportControllerSend(Clock* clock,
webrtc::RtcEventLog* event_log);
On 2017/03/27 11:14:11, stefan-webrtc wrote:
> Empty line below

Done.

https://codereview.webrtc.org/2685673003/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:111: // argument, and setting it later seems no-trivial.
On 2017/03/27 11:14:11, stefan-webrtc wrote:
> nit: non-trivial?
> 
> I think we later will want to be able to pass the congestion controller in, so
> maybe it doesn't even have to be owned by the transport controller later on.

Done.

[nisse-webrtc, 2017-03-27 11:55:52]

Description was changed from

==========
Define RtpTransportControllerSendInterface.

Implementation owned by call, and passed to VideoSendStream and
AudioSendStream.

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

to

==========
Define RtpTransportControllerSendInterface.

Implementation owned by call, and passed to VideoSendStream and
AudioSendStream.

BUG=webrtc:6847, webrtc:7135
==========

[nisse-webrtc, 2017-03-27 11:57:59]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-03-27 11:57:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2685673003/#ps300001 (title:
"Rebased.")

[commit-bot: I haz the power, 2017-03-27 11:58:04]

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-27 12:36:16]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1490615879005690,
"parent_rev": "0be49d8d1028a2eb3aafd979091cfa0311e6c3c3", "commit_rev":
"b8f9a324590b0c412c414ccaddfebd8aae12471f"}

[commit-bot: I haz the power, 2017-03-27 12:36:22]

Description was changed from

==========
Define RtpTransportControllerSendInterface.

Implementation owned by call, and passed to VideoSendStream and
AudioSendStream.

BUG=webrtc:6847, webrtc:7135
==========

to

==========
Define RtpTransportControllerSendInterface.

Implementation owned by call, and passed to VideoSendStream and
AudioSendStream.

BUG=webrtc:6847, webrtc:7135

Review-Url: https://codereview.webrtc.org/2685673003
Cr-Commit-Position: refs/heads/master@{#17389}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b8f9a324590b0c412c414ccad...
==========

[commit-bot: I haz the power, 2017-03-27 12:36:24]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/external/webrtc/+/b8f9a324590b0c412c414ccad...