Use separate rtp module lists for send and receive in PacketRouter.

Use separate rtp module lists for send and receive in PacketRouter.

This makes it possible to handle send and receive streams with the same SSRC, which is currently the case in some peer connection tests.

Also moves sending transport feedback to the pacer thread.

BUG=webrtc:5263
Committed: https://crrev.com/bba9dec4d57d8f2b7a3b67a61e63cc33667a2b54
Cr-Commit-Position: refs/heads/master@{#11443}

[stefan-webrtc, 2016-01-23 16:55:38]

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

[stefan-webrtc, 2016-01-23 16:55:38]

sprang: please review webrtc/video and webrtc/modules.

solenberg: please review webrtc/voice_engine/channel.cc.

[stefan-webrtc, 2016-01-23 16:57:35]

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

[commit-bot: I haz the power, 2016-01-23 16:57:38]

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

[tommi, 2016-01-23 17:23:26]

saturday drive-by

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:32: rtp_modules->erase(it);
if we didn't have to search the receiver list when removing a sender and vice
versa, then we wouldn't need to care if an item exists or not when removing it. 
We could just use std::list<>::remove(), but since there's no "bool sender"
argument to RemoveRtpModule, this is what we end up with.  What about adding a
bool flag to RemoveRtpModule too?

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:58: if (sender) {
is it good to lock both sender and receiver maps with the same lock?  I don't
suppose we can be smarter about when we lock and know about the threads that are
in the picture?

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:128: return true;
nit:

return webrtc::SendFeedback(packet, &recv_rtp_modules_) ||
       webrtc::SendFeedback(packet, &send_rtp_modules_);

it feels to me like the scope of the lock operations is very wide.  We
frequently hold the locks while calling out into other modules.

[stefan-webrtc, 2016-01-23 17:46:04]

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:32: rtp_modules->erase(it);
On 2016/01/23 17:23:26, tommi-webrtc wrote:
> if we didn't have to search the receiver list when removing a sender and vice
> versa, then we wouldn't need to care if an item exists or not when removing
it. 
> We could just use std::list<>::remove(), but since there's no "bool sender"
> argument to RemoveRtpModule, this is what we end up with.  What about adding a
> bool flag to RemoveRtpModule too?

Right, that would be an option, and I can definitely add a bool sender to the
remove method. The code before this CL also didn't do remove() though for the
reason that we want to be able to DCHECK that no modules are removed which
haven't also been added. Given this, do you still think it's worth doing as it
slightly complicates the API?

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:58: if (sender) {
On 2016/01/23 17:23:26, tommi-webrtc wrote:
> is it good to lock both sender and receiver maps with the same lock?  I don't
> suppose we can be smarter about when we lock and know about the threads that
are
> in the picture?

We could have different locks, but I don't think it would buy us any performance
since what this class does _should_ be very light weight.

I'll give the thread model some thought, but I think at least two threads are
involved here as video engine adds and removes modules in SetSendCodec which
will be called on the encoder thread in the near future.

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:128: return true;
On 2016/01/23 17:23:25, tommi-webrtc wrote:
> nit:
> 
> return webrtc::SendFeedback(packet, &recv_rtp_modules_) ||
>        webrtc::SendFeedback(packet, &send_rtp_modules_);
> 
> it feels to me like the scope of the lock operations is very wide.  We
> frequently hold the locks while calling out into other modules.

Agree, I think it would be better to find the module to send on and then release
the lock, which I think could work.

[commit-bot: I haz the power, 2016-01-23 17:57:53]

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

[commit-bot: I haz the power, 2016-01-23 17:57:53]

Dry run: This issue passed the CQ dry run.

[tommi, 2016-01-24 10:30:07]

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

[tommi, 2016-01-24 10:30:08]

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:32: rtp_modules->erase(it);
On 2016/01/23 17:46:03, stefan-webrtc (holmer) wrote:
> On 2016/01/23 17:23:26, tommi-webrtc wrote:
> > if we didn't have to search the receiver list when removing a sender and
vice
> > versa, then we wouldn't need to care if an item exists or not when removing
> it. 
> > We could just use std::list<>::remove(), but since there's no "bool sender"
> > argument to RemoveRtpModule, this is what we end up with.  What about adding
a
> > bool flag to RemoveRtpModule too?
> 
> Right, that would be an option, and I can definitely add a bool sender to the
> remove method. The code before this CL also didn't do remove() though for the
> reason that we want to be able to DCHECK that no modules are removed which
> haven't also been added. Given this, do you still think it's worth doing as it
> slightly complicates the API?

We can use std::find() inside an RTC_DCHECK to check if the pointer is in the
list.
If we don't want to add a bool to the Remove method, we could do this:

* Make AddRtpModule return a PacketRouter::ModuleRegistration type. (not a
pointer)
* The caller owns this registration instance and can call Unregister() on it if
explicit unregistration is needed, otherwise it will happen automatically in the
dtor.
* ModuleRegistration is a movable (implement ctor w/&&) but not copyable inner
class that has a 'bool sender_' variable and a pointer to the PacketRouter.
* Make PacketRouter::RemoveRtpModule private and only allow ModuleRegistration
to call it.
* In the dtor of ModuleRegistration, it calls
packet_router_->RemoveRtpModule(module_, sender_);

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:58: if (sender) {
On 2016/01/23 17:46:03, stefan-webrtc (holmer) wrote:
> On 2016/01/23 17:23:26, tommi-webrtc wrote:
> > is it good to lock both sender and receiver maps with the same lock?  I
don't
> > suppose we can be smarter about when we lock and know about the threads that
> are
> > in the picture?
> 
> We could have different locks, but I don't think it would buy us any
performance
> since what this class does _should_ be very light weight.
> 
> I'll give the thread model some thought, but I think at least two threads are
> involved here as video engine adds and removes modules in SetSendCodec which
> will be called on the encoder thread in the near future.

I think that this particular class is very light but it holds the lock while
calling out into other implementations.  Unregistering a receiver might have to
wait for a sender completing some operation that lies outside this class.  I
looked around for what's being done in some of the methods that we're calling
out to and it looks like there are many different implementations and hard for
me to say for sure that from a black box perspective calling the PacketRouter
methods from outside, is always going to be light weight, meaning low/no
contention.

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:128: return true;
On 2016/01/23 17:46:04, stefan-webrtc (holmer) wrote:
> On 2016/01/23 17:23:25, tommi-webrtc wrote:
> > nit:
> > 
> > return webrtc::SendFeedback(packet, &recv_rtp_modules_) ||
> >        webrtc::SendFeedback(packet, &send_rtp_modules_);
> > 
> > it feels to me like the scope of the lock operations is very wide.  We
> > frequently hold the locks while calling out into other modules.
> 
> Agree, I think it would be better to find the module to send on and then
release
> the lock, which I think could work.

No chance that it could be removed while the call is ongoing?  If we can make
such assumptions, I think we could reduce the lock scope in many methods.

[stefan-webrtc, 2016-01-25 11:52:25]

Addressed comments.

[stefan-webrtc, 2016-01-25 11:54:46]

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:32: rtp_modules->erase(it);
On 2016/01/24 10:30:08, tommi-webrtc wrote:
> On 2016/01/23 17:46:03, stefan-webrtc (holmer) wrote:
> > On 2016/01/23 17:23:26, tommi-webrtc wrote:
> > > if we didn't have to search the receiver list when removing a sender and
> vice
> > > versa, then we wouldn't need to care if an item exists or not when
removing
> > it. 
> > > We could just use std::list<>::remove(), but since there's no "bool
sender"
> > > argument to RemoveRtpModule, this is what we end up with.  What about
adding
> a
> > > bool flag to RemoveRtpModule too?
> > 
> > Right, that would be an option, and I can definitely add a bool sender to
the
> > remove method. The code before this CL also didn't do remove() though for
the
> > reason that we want to be able to DCHECK that no modules are removed which
> > haven't also been added. Given this, do you still think it's worth doing as
it
> > slightly complicates the API?
> 
> We can use std::find() inside an RTC_DCHECK to check if the pointer is in the
> list.
> If we don't want to add a bool to the Remove method, we could do this:
> 
> * Make AddRtpModule return a PacketRouter::ModuleRegistration type. (not a
> pointer)
> * The caller owns this registration instance and can call Unregister() on it
if
> explicit unregistration is needed, otherwise it will happen automatically in
the
> dtor.
> * ModuleRegistration is a movable (implement ctor w/&&) but not copyable inner
> class that has a 'bool sender_' variable and a pointer to the PacketRouter.
> * Make PacketRouter::RemoveRtpModule private and only allow ModuleRegistration
> to call it.
> * In the dtor of ModuleRegistration, it calls
> packet_router_->RemoveRtpModule(module_, sender_);

I think I'll just add a bool sender to RemoveRtpModule, should be easy enough
and avoids making the class complicated.

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:58: if (sender) {
On 2016/01/24 10:30:08, tommi-webrtc wrote:
> On 2016/01/23 17:46:03, stefan-webrtc (holmer) wrote:
> > On 2016/01/23 17:23:26, tommi-webrtc wrote:
> > > is it good to lock both sender and receiver maps with the same lock?  I
> don't
> > > suppose we can be smarter about when we lock and know about the threads
that
> > are
> > > in the picture?
> > 
> > We could have different locks, but I don't think it would buy us any
> performance
> > since what this class does _should_ be very light weight.
> > 
> > I'll give the thread model some thought, but I think at least two threads
are
> > involved here as video engine adds and removes modules in SetSendCodec which
> > will be called on the encoder thread in the near future.
> 
> I think that this particular class is very light but it holds the lock while
> calling out into other implementations.  Unregistering a receiver might have
to
> wait for a sender completing some operation that lies outside this class.  I
> looked around for what's being done in some of the methods that we're calling
> out to and it looks like there are many different implementations and hard for
> me to say for sure that from a black box perspective calling the PacketRouter
> methods from outside, is always going to be light weight, meaning low/no
> contention.

Agree it's not very clear how expensive it is to send packets. I'll split into
two crit sects as you suggest.

https://codereview.webrtc.org/1628683002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:128: return true;
On 2016/01/24 10:30:08, tommi-webrtc wrote:
> On 2016/01/23 17:46:04, stefan-webrtc (holmer) wrote:
> > On 2016/01/23 17:23:25, tommi-webrtc wrote:
> > > nit:
> > > 
> > > return webrtc::SendFeedback(packet, &recv_rtp_modules_) ||
> > >        webrtc::SendFeedback(packet, &send_rtp_modules_);
> > > 
> > > it feels to me like the scope of the lock operations is very wide.  We
> > > frequently hold the locks while calling out into other modules.
> > 
> > Agree, I think it would be better to find the module to send on and then
> release
> > the lock, which I think could work.
> 
> No chance that it could be removed while the call is ongoing?  If we can make
> such assumptions, I think we could reduce the lock scope in many methods.

I thought I could guarantee that, but unfortunately I can't.

[the sun, 2016-01-25 15:42:01]

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:127: if (::webrtc::SendFeedback(packet,
&recv_rtp_modules_))
I don't really follow this. Doesn't it make any difference which transport
feedback packets go out on?

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2933:
rtp_packet_sender_proxy_->HasPacketSender());
Can this property change as a consequence of this call to
SetCongestionControlObjects? Does it need to be evaluated twice - once before
SetPacketSender and once after?

[stefan-webrtc, 2016-01-25 16:00:26]

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:127: if (::webrtc::SendFeedback(packet,
&recv_rtp_modules_))
On 2016/01/25 15:42:01, the sun wrote:
> I don't really follow this. Doesn't it make any difference which transport
> feedback packets go out on?

No, since the feedback message is covering the whole call. You could say that we
shouldn't send feedback on an SSRC which hasn't been configured with transport
cc feedback, but we leave that decision to the rtp module.

In the future we will may add support for having separate BWE (and therefore
also feedback messages) for different transports (e.g., the non-BUNDLE case),
but that is not something we'll support in this first step.

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2933:
rtp_packet_sender_proxy_->HasPacketSender());
On 2016/01/25 15:42:01, the sun wrote:
> Can this property change as a consequence of this call to
> SetCongestionControlObjects? Does it need to be evaluated twice - once before
> SetPacketSender and once after?

It can change to null as the stream is being teared down, but a channel can only
be a sender or a receiver and this expression makes sure we can figure out which
it is. 

If rtp_packet_sender isn't null, we know that the channel (stream) is either
being teared down or that it is a receiving channel. If it's true we know that
it must be a sending channel since no receivers have an rtp_packet_sender.

To check the former case, we check if the rtp_packet_sender_proxy_ has already
been configured with an rtp_packet_sender, since in that case we know that it
already is a sending channel (and that it is actually being teared down).

Because of this I don't think we need to evaluate it twice.

[the sun, 2016-01-26 09:17:03]

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:127: if (::webrtc::SendFeedback(packet,
&recv_rtp_modules_))
On 2016/01/25 16:00:26, stefan-webrtc (holmer) wrote:
> On 2016/01/25 15:42:01, the sun wrote:
> > I don't really follow this. Doesn't it make any difference which transport
> > feedback packets go out on?
> 
> No, since the feedback message is covering the whole call. You could say that
we
> shouldn't send feedback on an SSRC which hasn't been configured with transport
> cc feedback, but we leave that decision to the rtp module.
> 
> In the future we will may add support for having separate BWE (and therefore
> also feedback messages) for different transports (e.g., the non-BUNDLE case),
> but that is not something we'll support in this first step.

So when would sending an RTCP packet fail? Won't we always use the first of the
available modules?

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.h (right):

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.h:59: // Map from ssrc to sending rtp
module.
Bad comment?

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2933:
rtp_packet_sender_proxy_->HasPacketSender());
On 2016/01/25 16:00:26, stefan-webrtc (holmer) wrote:
> On 2016/01/25 15:42:01, the sun wrote:
> > Can this property change as a consequence of this call to
> > SetCongestionControlObjects? Does it need to be evaluated twice - once
before
> > SetPacketSender and once after?
> 
> It can change to null as the stream is being teared down, but a channel can
only
> be a sender or a receiver and this expression makes sure we can figure out
which
> it is. 
> 
> If rtp_packet_sender isn't null, we know that the channel (stream) is either
> being teared down or that it is a receiving channel. If it's true we know that
> it must be a sending channel since no receivers have an rtp_packet_sender.
> 
> To check the former case, we check if the rtp_packet_sender_proxy_ has already
> been configured with an rtp_packet_sender, since in that case we know that it
> already is a sending channel (and that it is actually being teared down).
> 
> Because of this I don't think we need to evaluate it twice.

Very well, but I think this code is becoming hard to follow. How about we split
it into three functions instead?

SetSenderCongestionControlObjects(x, y, z)
SetReceiverCongestionControlObjects(z)
ResetCongestionControlObjects()

I think the logic would be simpler.

[stefan-webrtc, 2016-01-26 10:40:18]

Split SetCongestionControlObjects in three.

[stefan-webrtc, 2016-01-26 10:49:18]

Add more dchecks.

[stefan-webrtc, 2016-01-26 10:50:38]

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:127: if (::webrtc::SendFeedback(packet,
&recv_rtp_modules_))
On 2016/01/26 09:17:02, the sun wrote:
> On 2016/01/25 16:00:26, stefan-webrtc (holmer) wrote:
> > On 2016/01/25 15:42:01, the sun wrote:
> > > I don't really follow this. Doesn't it make any difference which transport
> > > feedback packets go out on?
> > 
> > No, since the feedback message is covering the whole call. You could say
that
> we
> > shouldn't send feedback on an SSRC which hasn't been configured with
transport
> > cc feedback, but we leave that decision to the rtp module.
> > 
> > In the future we will may add support for having separate BWE (and therefore
> > also feedback messages) for different transports (e.g., the non-BUNDLE
case),
> > but that is not something we'll support in this first step.
> 
> So when would sending an RTCP packet fail? Won't we always use the first of
the
> available modules?

Most of the time, yes. But it could be that in the future the first module
doesn't support cc feedback, or that the first module isn't currently enabled
for sending packets.

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.h (right):

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.h:59: // Map from ssrc to sending rtp
module.
On 2016/01/26 09:17:02, the sun wrote:
> Bad comment?

Done.

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1628683002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2933:
rtp_packet_sender_proxy_->HasPacketSender());
On 2016/01/26 09:17:03, the sun wrote:
> On 2016/01/25 16:00:26, stefan-webrtc (holmer) wrote:
> > On 2016/01/25 15:42:01, the sun wrote:
> > > Can this property change as a consequence of this call to
> > > SetCongestionControlObjects? Does it need to be evaluated twice - once
> before
> > > SetPacketSender and once after?
> > 
> > It can change to null as the stream is being teared down, but a channel can
> only
> > be a sender or a receiver and this expression makes sure we can figure out
> which
> > it is. 
> > 
> > If rtp_packet_sender isn't null, we know that the channel (stream) is either
> > being teared down or that it is a receiving channel. If it's true we know
that
> > it must be a sending channel since no receivers have an rtp_packet_sender.
> > 
> > To check the former case, we check if the rtp_packet_sender_proxy_ has
already
> > been configured with an rtp_packet_sender, since in that case we know that
it
> > already is a sending channel (and that it is actually being teared down).
> > 
> > Because of this I don't think we need to evaluate it twice.
> 
> Very well, but I think this code is becoming hard to follow. How about we
split
> it into three functions instead?
> 
> SetSenderCongestionControlObjects(x, y, z)
> SetReceiverCongestionControlObjects(z)
> ResetCongestionControlObjects()
> 
> I think the logic would be simpler.

Agree, done.

[sprang_webrtc, 2016-01-27 11:53:38]

lgtm for webrtc/video and webrtc/modules

[the sun, 2016-01-28 15:33:45]

Just one more thought.

Thanks for splitting on voe::Channel::SetCCObjects(), I think that turned out
very nicely!

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:79: if (rtp_module->SendingMedia() &&
ssrc == rtp_module->SSRC()) {
Can we rely on the RtpSender::sending_media_ flag to determine whether an
RtpRtcpModule is a sender? The flag is default inited to 'true' currently,
perhaps it should be false, and sending enabled only later.

Sorry to bring this up, but keeping the two lists of modules in the PacketRouter
seems pretty clumsy to me, both in terms of implementation and the PacketRouter
interface.

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/test/mock_voe_chan...
File webrtc/test/mock_voe_channel_proxy.h (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/test/mock_voe_chan...
webrtc/test/mock_voe_channel_proxy.h:37: void(PacketRouter* seq_num_allocator));
Can you change "seq_num_allocator" on both these methods into "packet_router",
please? Seems like an impl detail to me...

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2940:
packet_router->AddRtpModule(_rtpRtcpModule.get(), true);
Could we call _rtpRtcpModule->SetSendingMediaStatus(true) here? and ...(false)
in the method below?

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2953: if (feedback_observer_proxy_) {
This is always true? Do an RTC_DCHECK() like in RegisterSender...?
and for the ones below

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2962:
rtp_packet_sender_proxy_->SetPacketSender(nullptr);
Always just SetPacketSender(nullptr)? Remove HasPacketSender()

[stefan-webrtc, 2016-01-28 15:52:00]

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:79: if (rtp_module->SendingMedia() &&
ssrc == rtp_module->SSRC()) {
On 2016/01/28 15:33:45, the sun wrote:
> Can we rely on the RtpSender::sending_media_ flag to determine whether an
> RtpRtcpModule is a sender? The flag is default inited to 'true' currently,
> perhaps it should be false, and sending enabled only later.
> 
> Sorry to bring this up, but keeping the two lists of modules in the
PacketRouter
> seems pretty clumsy to me, both in terms of implementation and the
PacketRouter
> interface.

I'm not sure that is possible, as modules are added to the packet router before
they have been set as sending media, at least for video. For audio the rtp
modules are always set as sending media, probably because we aren't aware of
which modules are senders or not in channel.cc.

We can likely improve on this as you suggest, but I'd prefer keeping that
separate to not complicate this CL.

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/test/mock_voe_chan...
File webrtc/test/mock_voe_channel_proxy.h (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/test/mock_voe_chan...
webrtc/test/mock_voe_channel_proxy.h:37: void(PacketRouter* seq_num_allocator));
On 2016/01/28 15:33:45, the sun wrote:
> Can you change "seq_num_allocator" on both these methods into "packet_router",
> please? Seems like an impl detail to me...

Done.

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2940:
packet_router->AddRtpModule(_rtpRtcpModule.get(), true);
On 2016/01/28 15:33:45, the sun wrote:
> Could we call _rtpRtcpModule->SetSendingMediaStatus(true) here? and ...(false)
> in the method below?

I think we can, but as I'm not likely to be able to make use of it right now due
to how video code is written, I'd prefer to wait with that.

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2953: if (feedback_observer_proxy_) {
On 2016/01/28 15:33:45, the sun wrote:
> This is always true? Do an RTC_DCHECK() like in RegisterSender...?
> and for the ones below

No, it's not true if pacing hasn't been enabled for audio, which could be the
case for a receive stream. We could require that pacing is enabled for receive
streams too, but i think it would be a bit strange.

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2962:
rtp_packet_sender_proxy_->SetPacketSender(nullptr);
On 2016/01/28 15:33:45, the sun wrote:
> Always just SetPacketSender(nullptr)? Remove HasPacketSender()

I need HasPacketSender() to determine if it's a sender or not. Or would you like
me to get that status from the rtp module instead? That should work for voice I
think.

[stefan-webrtc, 2016-01-28 15:57:31]

Comment addressed.

[stefan-webrtc, 2016-01-28 15:59:49]

Rebase.

[the sun, 2016-01-29 10:07:07]

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:79: if (rtp_module->SendingMedia() &&
ssrc == rtp_module->SSRC()) {
On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> On 2016/01/28 15:33:45, the sun wrote:
> > Can we rely on the RtpSender::sending_media_ flag to determine whether an
> > RtpRtcpModule is a sender? The flag is default inited to 'true' currently,
> > perhaps it should be false, and sending enabled only later.
> > 
> > Sorry to bring this up, but keeping the two lists of modules in the
> PacketRouter
> > seems pretty clumsy to me, both in terms of implementation and the
> PacketRouter
> > interface.
> 
> I'm not sure that is possible, as modules are added to the packet router
before
> they have been set as sending media, at least for video. For audio the rtp
> modules are always set as sending media, probably because we aren't aware of
> which modules are senders or not in channel.cc.
> 
> We can likely improve on this as you suggest, but I'd prefer keeping that
> separate to not complicate this CL.

I thought it would actually make the CL simpler - we could remove the two lists
and basically keep the PacketRouter as before.

ViE disables SendingMedia when the modules are created:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

It seems VoE::Channel should do as ViE and enable SendingMedia in StartSend().

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2953: if (feedback_observer_proxy_) {
On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> On 2016/01/28 15:33:45, the sun wrote:
> > This is always true? Do an RTC_DCHECK() like in RegisterSender...?
> > and for the ones below
> 
> No, it's not true if pacing hasn't been enabled for audio, which could be the
> case for a receive stream. We could require that pacing is enabled for receive
> streams too, but i think it would be a bit strange.

Ah ok, very well. Another option would be to always initialize these members,
but leave them out from the RtpRtcp::Configuration unless pacing_enabled_. The
state of the object becomes slightly simpler then.

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2962:
rtp_packet_sender_proxy_->SetPacketSender(nullptr);
On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> On 2016/01/28 15:33:45, the sun wrote:
> > Always just SetPacketSender(nullptr)? Remove HasPacketSender()
> 
> I need HasPacketSender() to determine if it's a sender or not. Or would you
like
> me to get that status from the rtp module instead? That should work for voice
I
> think.

I think a single point of decision making is preferable, but best would be if we
didn't need the sender flag when calling RemoveRtpModule(). :)

[stefan-webrtc, 2016-01-29 10:58:09]

Comments addressed.

[stefan-webrtc, 2016-01-29 10:59:55]

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:79: if (rtp_module->SendingMedia() &&
ssrc == rtp_module->SSRC()) {
On 2016/01/29 10:07:07, the sun wrote:
> On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> > On 2016/01/28 15:33:45, the sun wrote:
> > > Can we rely on the RtpSender::sending_media_ flag to determine whether an
> > > RtpRtcpModule is a sender? The flag is default inited to 'true' currently,
> > > perhaps it should be false, and sending enabled only later.
> > > 
> > > Sorry to bring this up, but keeping the two lists of modules in the
> > PacketRouter
> > > seems pretty clumsy to me, both in terms of implementation and the
> > PacketRouter
> > > interface.
> > 
> > I'm not sure that is possible, as modules are added to the packet router
> before
> > they have been set as sending media, at least for video. For audio the rtp
> > modules are always set as sending media, probably because we aren't aware of
> > which modules are senders or not in channel.cc.
> > 
> > We can likely improve on this as you suggest, but I'd prefer keeping that
> > separate to not complicate this CL.
> 
> I thought it would actually make the CL simpler - we could remove the two
lists
> and basically keep the PacketRouter as before.
> 

I think we still should use two lists to reduce contention between send and
receive.

I really think we're better off trying to improve this in a future CL even
though it may mean having to change the API of this class. For instance we have
the possible issue of what would happen if we enable the module for sending
media before it actually has been added to the packet router. We may end up not
routing the first packets and thereby silently dropping them.

> ViE disables SendingMedia when the modules are created:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> It seems VoE::Channel should do as ViE and enable SendingMedia in StartSend().

Yes, that is possible. Out of scope for this CL though I think.

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2953: if (feedback_observer_proxy_) {
On 2016/01/29 10:07:07, the sun wrote:
> On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> > On 2016/01/28 15:33:45, the sun wrote:
> > > This is always true? Do an RTC_DCHECK() like in RegisterSender...?
> > > and for the ones below
> > 
> > No, it's not true if pacing hasn't been enabled for audio, which could be
the
> > case for a receive stream. We could require that pacing is enabled for
receive
> > streams too, but i think it would be a bit strange.
> 
> Ah ok, very well. Another option would be to always initialize these members,
> but leave them out from the RtpRtcp::Configuration unless pacing_enabled_. The
> state of the object becomes slightly simpler then.

That's an option. I'll make it so.

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2962:
rtp_packet_sender_proxy_->SetPacketSender(nullptr);
On 2016/01/29 10:07:07, the sun wrote:
> On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> > On 2016/01/28 15:33:45, the sun wrote:
> > > Always just SetPacketSender(nullptr)? Remove HasPacketSender()
> > 
> > I need HasPacketSender() to determine if it's a sender or not. Or would you
> like
> > me to get that status from the rtp module instead? That should work for
voice
> I
> > think.
> 
> I think a single point of decision making is preferable, but best would be if
we
> didn't need the sender flag when calling RemoveRtpModule(). :)

Ack. I think it will be hard to do at the moment unfortunately. Do you want me
to make a change here?

[the sun, 2016-01-29 12:29:43]

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:79: if (rtp_module->SendingMedia() &&
ssrc == rtp_module->SSRC()) {
On 2016/01/29 10:59:55, stefan-webrtc (holmer) wrote:
> On 2016/01/29 10:07:07, the sun wrote:
> > On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> > > On 2016/01/28 15:33:45, the sun wrote:
> > > > Can we rely on the RtpSender::sending_media_ flag to determine whether
an
> > > > RtpRtcpModule is a sender? The flag is default inited to 'true'
currently,
> > > > perhaps it should be false, and sending enabled only later.
> > > > 
> > > > Sorry to bring this up, but keeping the two lists of modules in the
> > > PacketRouter
> > > > seems pretty clumsy to me, both in terms of implementation and the
> > > PacketRouter
> > > > interface.
> > > 
> > > I'm not sure that is possible, as modules are added to the packet router
> > before
> > > they have been set as sending media, at least for video. For audio the rtp
> > > modules are always set as sending media, probably because we aren't aware
of
> > > which modules are senders or not in channel.cc.
> > > 
> > > We can likely improve on this as you suggest, but I'd prefer keeping that
> > > separate to not complicate this CL.
> > 
> > I thought it would actually make the CL simpler - we could remove the two
> lists
> > and basically keep the PacketRouter as before.
> > 
> 
> I think we still should use two lists to reduce contention between send and
> receive.

Did you ever figure out how many threads are involved here and the amount of
contention there really is?

To me it looks like the SendFeedback() is called from
RemoteEstimatorProxy::Process() and TimeToSendPacket()/TimeToSendPadding() are
called from PacedSender::Process(), so the competition would then be with the
threads adding/removing the modules.

For audio that would be the worker thread creating/deleting send/recv streams,
and for video in response to VideoSendStream::ReconfigureVideoEncoder(), which
happens on either the worker thread or the capture thread, but only if video
dimensions change.

It thus seems like contention is pretty rare here?

> 
> I really think we're better off trying to improve this in a future CL even
> though it may mean having to change the API of this class. For instance we
have
> the possible issue of what would happen if we enable the module for sending
> media before it actually has been added to the packet router. We may end up
not
> routing the first packets and thereby silently dropping them.

Ok, now I see what you mean, ViEChannel::SetSendCodec() isn't that
straightforward to simplify.

> > ViE disables SendingMedia when the modules are created:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> > 
> > It seems VoE::Channel should do as ViE and enable SendingMedia in
StartSend().
> 
> Yes, that is possible. Out of scope for this CL though I think.

[stefan-webrtc, 2016-01-29 13:36:33]

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:79: if (rtp_module->SendingMedia() &&
ssrc == rtp_module->SSRC()) {
On 2016/01/29 12:29:43, the sun wrote:
> On 2016/01/29 10:59:55, stefan-webrtc (holmer) wrote:
> > On 2016/01/29 10:07:07, the sun wrote:
> > > On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> > > > On 2016/01/28 15:33:45, the sun wrote:
> > > > > Can we rely on the RtpSender::sending_media_ flag to determine whether
> an
> > > > > RtpRtcpModule is a sender? The flag is default inited to 'true'
> currently,
> > > > > perhaps it should be false, and sending enabled only later.
> > > > > 
> > > > > Sorry to bring this up, but keeping the two lists of modules in the
> > > > PacketRouter
> > > > > seems pretty clumsy to me, both in terms of implementation and the
> > > > PacketRouter
> > > > > interface.
> > > > 
> > > > I'm not sure that is possible, as modules are added to the packet router
> > > before
> > > > they have been set as sending media, at least for video. For audio the
rtp
> > > > modules are always set as sending media, probably because we aren't
aware
> of
> > > > which modules are senders or not in channel.cc.
> > > > 
> > > > We can likely improve on this as you suggest, but I'd prefer keeping
that
> > > > separate to not complicate this CL.
> > > 
> > > I thought it would actually make the CL simpler - we could remove the two
> > lists
> > > and basically keep the PacketRouter as before.
> > > 
> > 
> > I think we still should use two lists to reduce contention between send and
> > receive.
> 
> Did you ever figure out how many threads are involved here and the amount of
> contention there really is?
> 
> To me it looks like the SendFeedback() is called from
> RemoteEstimatorProxy::Process() and TimeToSendPacket()/TimeToSendPadding() are
> called from PacedSender::Process(), so the competition would then be with the
> threads adding/removing the modules.
> 
> For audio that would be the worker thread creating/deleting send/recv streams,
> and for video in response to VideoSendStream::ReconfigureVideoEncoder(), which
> happens on either the worker thread or the capture thread, but only if video
> dimensions change.
> 
> It thus seems like contention is pretty rare here?

Possibly, but both TimeToSendPacket() and SendFeedback() call out from webrtc to
the transport. This should in most cases be fairly cheap assuming we have
non-blocking sockets, but I'm not sure we should make that assumption here? I
don't feel strongly in either direction, both have pros and cons. The way it is
now we are at least fairly sure we don't cause perf issues.

Do you prefer the other way?

> 
> > 
> > I really think we're better off trying to improve this in a future CL even
> > though it may mean having to change the API of this class. For instance we
> have
> > the possible issue of what would happen if we enable the module for sending
> > media before it actually has been added to the packet router. We may end up
> not
> > routing the first packets and thereby silently dropping them.
> 
> Ok, now I see what you mean, ViEChannel::SetSendCodec() isn't that
> straightforward to simplify.
> 
> > > ViE disables SendingMedia when the modules are created:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> > > 
> > > It seems VoE::Channel should do as ViE and enable SendingMedia in
> StartSend().
> > 
> > Yes, that is possible. Out of scope for this CL though I think.

[the sun, 2016-01-29 15:38:15]

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:79: if (rtp_module->SendingMedia() &&
ssrc == rtp_module->SSRC()) {
On 2016/01/29 13:36:33, stefan-webrtc (holmer) wrote:
> On 2016/01/29 12:29:43, the sun wrote:
> > On 2016/01/29 10:59:55, stefan-webrtc (holmer) wrote:
> > > On 2016/01/29 10:07:07, the sun wrote:
> > > > On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> > > > > On 2016/01/28 15:33:45, the sun wrote:
> > > > > > Can we rely on the RtpSender::sending_media_ flag to determine
whether
> > an
> > > > > > RtpRtcpModule is a sender? The flag is default inited to 'true'
> > currently,
> > > > > > perhaps it should be false, and sending enabled only later.
> > > > > > 
> > > > > > Sorry to bring this up, but keeping the two lists of modules in the
> > > > > PacketRouter
> > > > > > seems pretty clumsy to me, both in terms of implementation and the
> > > > > PacketRouter
> > > > > > interface.
> > > > > 
> > > > > I'm not sure that is possible, as modules are added to the packet
router
> > > > before
> > > > > they have been set as sending media, at least for video. For audio the
> rtp
> > > > > modules are always set as sending media, probably because we aren't
> aware
> > of
> > > > > which modules are senders or not in channel.cc.
> > > > > 
> > > > > We can likely improve on this as you suggest, but I'd prefer keeping
> that
> > > > > separate to not complicate this CL.
> > > > 
> > > > I thought it would actually make the CL simpler - we could remove the
two
> > > lists
> > > > and basically keep the PacketRouter as before.
> > > > 
> > > 
> > > I think we still should use two lists to reduce contention between send
and
> > > receive.
> > 
> > Did you ever figure out how many threads are involved here and the amount of
> > contention there really is?
> > 
> > To me it looks like the SendFeedback() is called from
> > RemoteEstimatorProxy::Process() and TimeToSendPacket()/TimeToSendPadding()
are
> > called from PacedSender::Process(), so the competition would then be with
the
> > threads adding/removing the modules.
> > 
> > For audio that would be the worker thread creating/deleting send/recv
streams,
> > and for video in response to VideoSendStream::ReconfigureVideoEncoder(),
which
> > happens on either the worker thread or the capture thread, but only if video
> > dimensions change.
> > 
> > It thus seems like contention is pretty rare here?
> 
> Possibly, but both TimeToSendPacket() and SendFeedback() call out from webrtc
to
> the transport. This should in most cases be fairly cheap assuming we have
> non-blocking sockets, but I'm not sure we should make that assumption here? I
> don't feel strongly in either direction, both have pros and cons. The way it
is
> now we are at least fairly sure we don't cause perf issues.

Sorry, I may have missed something, are you saying we have multiple process
threads? AFAICT TimeToSendPacket(), SendFeedback() and TimeToSendPadding() are
all called from the Process() method of either the PacedSender or
RemoteEstimatorProxy. I was assuming we only have one process thread, meaning
then there would be no competition between those calls.

Or are you concerned about the delay sending a packet on the network may cause
for adding/reconfiguring streams?

> Do you prefer the other way?

I prefer the simple and efficient way, whichever that is. Help me see.

> > 
> > > 
> > > I really think we're better off trying to improve this in a future CL even
> > > though it may mean having to change the API of this class. For instance we
> > have
> > > the possible issue of what would happen if we enable the module for
sending
> > > media before it actually has been added to the packet router. We may end
up
> > not
> > > routing the first packets and thereby silently dropping them.
> > 
> > Ok, now I see what you mean, ViEChannel::SetSendCodec() isn't that
> > straightforward to simplify.
> > 
> > > > ViE disables SendingMedia when the modules are created:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> > > > 
> > > > It seems VoE::Channel should do as ViE and enable SendingMedia in
> > StartSend().
> > > 
> > > Yes, that is possible. Out of scope for this CL though I think.
>

[stefan-webrtc, 2016-01-29 16:09:07]

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:79: if (rtp_module->SendingMedia() &&
ssrc == rtp_module->SSRC()) {
On 2016/01/29 15:38:14, the sun wrote:
> On 2016/01/29 13:36:33, stefan-webrtc (holmer) wrote:
> > On 2016/01/29 12:29:43, the sun wrote:
> > > On 2016/01/29 10:59:55, stefan-webrtc (holmer) wrote:
> > > > On 2016/01/29 10:07:07, the sun wrote:
> > > > > On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> > > > > > On 2016/01/28 15:33:45, the sun wrote:
> > > > > > > Can we rely on the RtpSender::sending_media_ flag to determine
> whether
> > > an
> > > > > > > RtpRtcpModule is a sender? The flag is default inited to 'true'
> > > currently,
> > > > > > > perhaps it should be false, and sending enabled only later.
> > > > > > > 
> > > > > > > Sorry to bring this up, but keeping the two lists of modules in
the
> > > > > > PacketRouter
> > > > > > > seems pretty clumsy to me, both in terms of implementation and the
> > > > > > PacketRouter
> > > > > > > interface.
> > > > > > 
> > > > > > I'm not sure that is possible, as modules are added to the packet
> router
> > > > > before
> > > > > > they have been set as sending media, at least for video. For audio
the
> > rtp
> > > > > > modules are always set as sending media, probably because we aren't
> > aware
> > > of
> > > > > > which modules are senders or not in channel.cc.
> > > > > > 
> > > > > > We can likely improve on this as you suggest, but I'd prefer keeping
> > that
> > > > > > separate to not complicate this CL.
> > > > > 
> > > > > I thought it would actually make the CL simpler - we could remove the
> two
> > > > lists
> > > > > and basically keep the PacketRouter as before.
> > > > > 
> > > > 
> > > > I think we still should use two lists to reduce contention between send
> and
> > > > receive.
> > > 
> > > Did you ever figure out how many threads are involved here and the amount
of
> > > contention there really is?
> > > 
> > > To me it looks like the SendFeedback() is called from
> > > RemoteEstimatorProxy::Process() and TimeToSendPacket()/TimeToSendPadding()
> are
> > > called from PacedSender::Process(), so the competition would then be with
> the
> > > threads adding/removing the modules.
> > > 
> > > For audio that would be the worker thread creating/deleting send/recv
> streams,
> > > and for video in response to VideoSendStream::ReconfigureVideoEncoder(),
> which
> > > happens on either the worker thread or the capture thread, but only if
video
> > > dimensions change.
> > > 
> > > It thus seems like contention is pretty rare here?
> > 
> > Possibly, but both TimeToSendPacket() and SendFeedback() call out from
webrtc
> to
> > the transport. This should in most cases be fairly cheap assuming we have
> > non-blocking sockets, but I'm not sure we should make that assumption here?
I
> > don't feel strongly in either direction, both have pros and cons. The way it
> is
> > now we are at least fairly sure we don't cause perf issues.
> 
> Sorry, I may have missed something, are you saying we have multiple process
> threads? AFAICT TimeToSendPacket(), SendFeedback() and TimeToSendPadding() are
> all called from the Process() method of either the PacedSender or
> RemoteEstimatorProxy. I was assuming we only have one process thread, meaning
> then there would be no competition between those calls.
> 
> Or are you concerned about the delay sending a packet on the network may cause
> for adding/reconfiguring streams?

Yes there are two, one for sending packets and (called from pacer) and one for
general processing, e.g. producing feedback.

It's probably not unreasonable to think feedback may be moved to the pacer
thread at some point though, but I'm not entirely sure about that.

Given this, and the fact that both will anyway be contending for the same socket
most of the time, I think we may be OK with a single lock after all. Let me know
if you agree and I'll revert some of the changes here.


> 
> > Do you prefer the other way?
> 
> I prefer the simple and efficient way, whichever that is. Help me see.
> 
> > > 
> > > > 
> > > > I really think we're better off trying to improve this in a future CL
even
> > > > though it may mean having to change the API of this class. For instance
we
> > > have
> > > > the possible issue of what would happen if we enable the module for
> sending
> > > > media before it actually has been added to the packet router. We may end
> up
> > > not
> > > > routing the first packets and thereby silently dropping them.
> > > 
> > > Ok, now I see what you mean, ViEChannel::SetSendCodec() isn't that
> > > straightforward to simplify.
> > > 
> > > > > ViE disables SendingMedia when the modules are created:
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> > > > > 
> > > > > It seems VoE::Channel should do as ViE and enable SendingMedia in
> > > StartSend().
> > > > 
> > > > Yes, that is possible. Out of scope for this CL though I think.
> >

[the sun, 2016-01-29 17:15:11]

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1628683002/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:79: if (rtp_module->SendingMedia() &&
ssrc == rtp_module->SSRC()) {
On 2016/01/29 16:09:07, stefan-webrtc (holmer) wrote:
> On 2016/01/29 15:38:14, the sun wrote:
> > On 2016/01/29 13:36:33, stefan-webrtc (holmer) wrote:
> > > On 2016/01/29 12:29:43, the sun wrote:
> > > > On 2016/01/29 10:59:55, stefan-webrtc (holmer) wrote:
> > > > > On 2016/01/29 10:07:07, the sun wrote:
> > > > > > On 2016/01/28 15:52:00, stefan-webrtc (holmer) wrote:
> > > > > > > On 2016/01/28 15:33:45, the sun wrote:
> > > > > > > > Can we rely on the RtpSender::sending_media_ flag to determine
> > whether
> > > > an
> > > > > > > > RtpRtcpModule is a sender? The flag is default inited to 'true'
> > > > currently,
> > > > > > > > perhaps it should be false, and sending enabled only later.
> > > > > > > > 
> > > > > > > > Sorry to bring this up, but keeping the two lists of modules in
> the
> > > > > > > PacketRouter
> > > > > > > > seems pretty clumsy to me, both in terms of implementation and
the
> > > > > > > PacketRouter
> > > > > > > > interface.
> > > > > > > 
> > > > > > > I'm not sure that is possible, as modules are added to the packet
> > router
> > > > > > before
> > > > > > > they have been set as sending media, at least for video. For audio
> the
> > > rtp
> > > > > > > modules are always set as sending media, probably because we
aren't
> > > aware
> > > > of
> > > > > > > which modules are senders or not in channel.cc.
> > > > > > > 
> > > > > > > We can likely improve on this as you suggest, but I'd prefer
keeping
> > > that
> > > > > > > separate to not complicate this CL.
> > > > > > 
> > > > > > I thought it would actually make the CL simpler - we could remove
the
> > two
> > > > > lists
> > > > > > and basically keep the PacketRouter as before.
> > > > > > 
> > > > > 
> > > > > I think we still should use two lists to reduce contention between
send
> > and
> > > > > receive.
> > > > 
> > > > Did you ever figure out how many threads are involved here and the
amount
> of
> > > > contention there really is?
> > > > 
> > > > To me it looks like the SendFeedback() is called from
> > > > RemoteEstimatorProxy::Process() and
TimeToSendPacket()/TimeToSendPadding()
> > are
> > > > called from PacedSender::Process(), so the competition would then be
with
> > the
> > > > threads adding/removing the modules.
> > > > 
> > > > For audio that would be the worker thread creating/deleting send/recv
> > streams,
> > > > and for video in response to VideoSendStream::ReconfigureVideoEncoder(),
> > which
> > > > happens on either the worker thread or the capture thread, but only if
> video
> > > > dimensions change.
> > > > 
> > > > It thus seems like contention is pretty rare here?
> > > 
> > > Possibly, but both TimeToSendPacket() and SendFeedback() call out from
> webrtc
> > to
> > > the transport. This should in most cases be fairly cheap assuming we have
> > > non-blocking sockets, but I'm not sure we should make that assumption
here?
> I
> > > don't feel strongly in either direction, both have pros and cons. The way
it
> > is
> > > now we are at least fairly sure we don't cause perf issues.
> > 
> > Sorry, I may have missed something, are you saying we have multiple process
> > threads? AFAICT TimeToSendPacket(), SendFeedback() and TimeToSendPadding()
are
> > all called from the Process() method of either the PacedSender or
> > RemoteEstimatorProxy. I was assuming we only have one process thread,
meaning
> > then there would be no competition between those calls.
> > 
> > Or are you concerned about the delay sending a packet on the network may
cause
> > for adding/reconfiguring streams?
> 
> Yes there are two, one for sending packets and (called from pacer) and one for
> general processing, e.g. producing feedback.

Ah, now I understand!

> It's probably not unreasonable to think feedback may be moved to the pacer
> thread at some point though, but I'm not entirely sure about that.
> 
> Given this, and the fact that both will anyway be contending for the same
socket
> most of the time, I think we may be OK with a single lock after all. Let me
know
> if you agree and I'll revert some of the changes here.

Regardless of the socket, there's a common choke in the MediaChannel base class:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

So between feedback/media packets, having two locks in PacketRouter doesn't
accomplish much. In the case where audio/video go to different sockets, some
contention may be introduced because of a single lock in PacketRouter. I think
it'd be nice if we could keep a single list of modules here and make sure
SendingMedia() is set up correctly for voice channels. That'd be closer to
solving the real problem. But I understand the original reasoning now so I leave
it to you to decide.

> 
> > 
> > > Do you prefer the other way?
> > 
> > I prefer the simple and efficient way, whichever that is. Help me see.
> > 
> > > > 
> > > > > 
> > > > > I really think we're better off trying to improve this in a future CL
> even
> > > > > though it may mean having to change the API of this class. For
instance
> we
> > > > have
> > > > > the possible issue of what would happen if we enable the module for
> > sending
> > > > > media before it actually has been added to the packet router. We may
end
> > up
> > > > not
> > > > > routing the first packets and thereby silently dropping them.
> > > > 
> > > > Ok, now I see what you mean, ViEChannel::SetSendCodec() isn't that
> > > > straightforward to simplify.
> > > > 
> > > > > > ViE disables SendingMedia when the modules are created:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> > > > > > 
> > > > > > It seems VoE::Channel should do as ViE and enable SendingMedia in
> > > > StartSend().
> > > > > 
> > > > > Yes, that is possible. Out of scope for this CL though I think.
> > > 
>

[stefan-webrtc, 2016-02-01 10:02:06]

Moved feedback to pacer thread.

[stefan-webrtc, 2016-02-01 10:05:51]

PTAL

I moved SendFeedback() to the pacer thread to make sure all packets are sent on
the same thread, which makes a lot of sense. Thanks for the suggestion Tommi.

I think this is as far as we can go with this CL without making it a lot larger
and more complicated.

[the sun, 2016-02-01 10:34:18]

lgtm % nits

https://codereview.webrtc.org/1628683002/diff/140001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1628683002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:2594: RTC_DCHECK(packet_router &&
!packet_router_);
nit: put the dchecks in the same order as args

https://codereview.webrtc.org/1628683002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:2601:
_rtpRtcpModule->SetStorePacketsStatus(rtp_packet_sender != nullptr, 600);
replace "rtp_packet_sender != nullptr" with "true"

[stefan-webrtc, 2016-02-01 10:36:24]

https://codereview.webrtc.org/1628683002/diff/140001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1628683002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:2594: RTC_DCHECK(packet_router &&
!packet_router_);
On 2016/02/01 10:34:18, the sun wrote:
> nit: put the dchecks in the same order as args

Done.

https://codereview.webrtc.org/1628683002/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:2601:
_rtpRtcpModule->SetStorePacketsStatus(rtp_packet_sender != nullptr, 600);
On 2016/02/01 10:34:18, the sun wrote:
> replace "rtp_packet_sender != nullptr" with "true"

Done.

[stefan-webrtc, 2016-02-01 10:36:44]

Comments addressed.

[stefan-webrtc, 2016-02-01 10:44:55]

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

[commit-bot: I haz the power, 2016-02-01 10:45:04]

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

[stefan-webrtc, 2016-02-01 10:52:15]

Description was changed from

==========
Use separate rtp module lists for send and receive in PacketRouter.

This makes it possible to handle send and receive streams with the same SSRC,
which is currently the case in some peer connection tests.

BUG=webrtc:5263
==========

to

==========
Use separate rtp module lists for send and receive in PacketRouter.

This makes it possible to handle send and receive streams with the same SSRC,
which is currently the case in some peer connection tests.

Also moves sending transport feedback to the pacer thread.

BUG=webrtc:5263
==========

[commit-bot: I haz the power, 2016-02-01 12:21:55]

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

[commit-bot: I haz the power, 2016-02-01 12:21:56]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-02-01 12:38:28]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2016-02-01 12:38:28]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1628683002/#ps160001 (title:
"Comments addressed.")

[commit-bot: I haz the power, 2016-02-01 12:38:39]

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

[commit-bot: I haz the power, 2016-02-01 12:40:00]

Description was changed from

==========
Use separate rtp module lists for send and receive in PacketRouter.

This makes it possible to handle send and receive streams with the same SSRC,
which is currently the case in some peer connection tests.

Also moves sending transport feedback to the pacer thread.

BUG=webrtc:5263
==========

to

==========
Use separate rtp module lists for send and receive in PacketRouter.

This makes it possible to handle send and receive streams with the same SSRC,
which is currently the case in some peer connection tests.

Also moves sending transport feedback to the pacer thread.

BUG=webrtc:5263
==========

[commit-bot: I haz the power, 2016-02-01 12:40:01]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-01 12:40:11]

Description was changed from

==========
Use separate rtp module lists for send and receive in PacketRouter.

This makes it possible to handle send and receive streams with the same SSRC,
which is currently the case in some peer connection tests.

Also moves sending transport feedback to the pacer thread.

BUG=webrtc:5263
==========

to

==========
Use separate rtp module lists for send and receive in PacketRouter.

This makes it possible to handle send and receive streams with the same SSRC,
which is currently the case in some peer connection tests.

Also moves sending transport feedback to the pacer thread.

BUG=webrtc:5263

Committed: https://crrev.com/bba9dec4d57d8f2b7a3b67a61e63cc33667a2b54
Cr-Commit-Position: refs/heads/master@{#11443}
==========

[commit-bot: I haz the power, 2016-02-01 12:40:12]

Patchset 9 (id:??) landed as
https://crrev.com/bba9dec4d57d8f2b7a3b67a61e63cc33667a2b54
Cr-Commit-Position: refs/heads/master@{#11443}

[the sun, 2016-02-02 13:58:57]

On 2016/02/01 12:40:12, commit-bot: I haz the power wrote:
> Patchset 9 (id:??) landed as
> https://crrev.com/bba9dec4d57d8f2b7a3b67a61e63cc33667a2b54
> Cr-Commit-Position: refs/heads/master@{#11443}

Did you open a bug to remind about the discussed cleanup of how SendingMedia is
used, so we can rely on that flag instead of using two module lists in the
PacketRouter?

[stefan-webrtc, 2016-02-04 09:06:57]

On 2016/02/02 13:58:57, the sun wrote:
> On 2016/02/01 12:40:12, commit-bot: I haz the power wrote:
> > Patchset 9 (id:??) landed as
> > https://crrev.com/bba9dec4d57d8f2b7a3b67a61e63cc33667a2b54
> > Cr-Commit-Position: refs/heads/master@{#11443}
> 
> Did you open a bug to remind about the discussed cleanup of how SendingMedia
is
> used, so we can rely on that flag instead of using two module lists in the
> PacketRouter?

Done. https://bugs.chromium.org/p/webrtc/issues/detail?id=5489