Let PacketRouter separate send and receive modules.

Let PacketRouter separate send and receive modules.

This is in preparation for merging the ViERemb logic in packet_router,
to send REMB feedback as sender reports if possible, otherwise as
receiver reports.

BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2774623006
Cr-Commit-Position: refs/heads/master@{#17489}
Committed: https://chromium.googlesource.com/external/webrtc/+/fdbfdc9786de5ec50ff750e5a61c3d39524e24bb

[nisse-webrtc, 2017-03-24 08:44:59]

nisse@webrtc.org changed reviewers:
+ mflodman@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2017-03-24 08:44:59]

PTAL. This let's the packet router keep separate lists of send and receive
modules.

Fredrik, what's your plan for voe::Channel? Kept as is for now, or delete, or
split into send and receive channels?

My plan is to merge the ViERemb logic in packet router.

But I think it would be even better of we could let packet_router handle the
send modules only (since it belongs to the pacer) and move list of receive
modules elsewhere. Then pacer and packet router would both belong to the
implementation of RtpTransportControllerSendInterface, and "elsewhere" would be
some object associated with RtpTransportControllerReceiveInterface. 

As for REMB, we plan to discuss that on Monday, 
if we can put the REMB feedback in receiver reports always (which would be
logical), or if we need to keep the current logic which prefers to attach REMB
feedback in sender reports.

PS. I now realize that I got SendFeedback wrong in the first patchset.
SendFeedback is for echoing back received transport sequence numbers, right? So
it's a receive side thing, and should use a receive module, not send module.
Agree? I'll fix in next patch set.

[the sun, 2017-03-24 09:59:49]

On 2017/03/24 08:44:59, nisse-webrtc wrote:
> PTAL. This let's the packet router keep separate lists of send and receive
> modules.
> 
> Fredrik, what's your plan for voe::Channel? Kept as is for now, or delete, or
> split into send and receive channels?

The plan is to let Audio[Send|Receive]Stream gradually "eat" their parts of
voe::Channel. Ideally, we'd have that generic transport interface so we could
remove any direct knowledge of RTP so you could keep these things contained
within the RTP implementation of such a transport.

> My plan is to merge the ViERemb logic in packet router.
> 
> But I think it would be even better of we could let packet_router handle the
> send modules only (since it belongs to the pacer) and move list of receive
> modules elsewhere. Then pacer and packet router would both belong to the
> implementation of RtpTransportControllerSendInterface, and "elsewhere" would
be
> some object associated with RtpTransportControllerReceiveInterface. 
> 
> As for REMB, we plan to discuss that on Monday, 
> if we can put the REMB feedback in receiver reports always (which would be
> logical), or if we need to keep the current logic which prefers to attach REMB
> feedback in sender reports.
> 
> PS. I now realize that I got SendFeedback wrong in the first patchset.
> SendFeedback is for echoing back received transport sequence numbers, right?
So
> it's a receive side thing, and should use a receive module, not send module.
> Agree? I'll fix in next patch set.

[nisse-webrtc, 2017-03-24 15:06:07]

This seems to pass try bots now.

[the sun, 2017-03-28 12:21:52]

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:124: rtc::CritScope cs(&modules_crit_);
From which thread is this driven, vs TimeToSendPacket()/TimeToSendPadding()?
Will there be contention on this lock?

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.h (right):

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.h:72: std::list<RtpRtcp*>
rtp_receive_modules_ GUARDED_BY(modules_crit_);
Any particular reason to use std::list? More cache jumping when iterating over,
vs vector.

[nisse-webrtc, 2017-03-29 13:58:37]

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:124: rtc::CritScope cs(&modules_crit_);
On 2017/03/28 12:21:52, the sun wrote:
> From which thread is this driven, vs TimeToSendPacket()/TimeToSendPadding()?
> Will there be contention on this lock?

Not entirely sure, but I think TimeToSendPacket is |Call::pacer_thread_|, and
SendFeedback is |Call::module_process_thread_|.

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.h (right):

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.h:72: std::list<RtpRtcp*>
rtp_receive_modules_ GUARDED_BY(modules_crit_);
On 2017/03/28 12:21:52, the sun wrote:
> Any particular reason to use std::list? More cache jumping when iterating
over,
> vs vector.

For the list of send modules, the code uses both push_back and push_front. The
list of receive modules could be changed to vector.

[the sun, 2017-03-31 07:17:14]

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:124: rtc::CritScope cs(&modules_crit_);
On 2017/03/29 13:58:36, nisse-webrtc wrote:
> On 2017/03/28 12:21:52, the sun wrote:
> > From which thread is this driven, vs TimeToSendPacket()/TimeToSendPadding()?
> > Will there be contention on this lock?
> 
> Not entirely sure, but I think TimeToSendPacket is |Call::pacer_thread_|, and
> SendFeedback is |Call::module_process_thread_|.

Could you add the missing thread checkers to document this?

https://codereview.webrtc.org/2774623006/diff/140001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.h (right):

https://codereview.webrtc.org/2774623006/diff/140001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.h:32: // on the simulcast layer in
RTPVideoHeader.
This comment doesn't make sense to me. The class "seems" to be used for audio as
well. There is no reference to RTPVideoHeader in this file or in the audio
stream classes.

https://codereview.webrtc.org/2774623006/diff/140001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2774623006/diff/140001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:258: }
I don't see a test where you have both send+receive modules, to test
PacketRouter::SendFeedback().

[nisse-webrtc, 2017-03-31 08:30:54]

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:124: rtc::CritScope cs(&modules_crit_);
On 2017/03/29 13:58:36, nisse-webrtc wrote:
> On 2017/03/28 12:21:52, the sun wrote:
> > From which thread is this driven, vs TimeToSendPacket()/TimeToSendPadding()?
> > Will there be contention on this lock?
> 
> Not entirely sure, but I think TimeToSendPacket is |Call::pacer_thread_|, and
> SendFeedback is |Call::module_process_thread_|.

After a second look, I think SendFeedback is actually called by the pacer
thread. In which case the lock shouldn't be necessary. I'll test that out in a
separate cl.

[nisse-webrtc, 2017-03-31 09:41:32]

New version.

https://codereview.webrtc.org/2774623006/diff/140001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.h (right):

https://codereview.webrtc.org/2774623006/diff/140001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.h:32: // on the simulcast layer in
RTPVideoHeader.
On 2017/03/31 07:17:13, the sun wrote:
> This comment doesn't make sense to me. The class "seems" to be used for audio
as
> well. There is no reference to RTPVideoHeader in this file or in the audio
> stream classes.

I'm rewriting the comment. Stefan, do you know the history, and when simulcast
logic was moved elsewhere?

https://codereview.webrtc.org/2774623006/diff/140001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2774623006/diff/140001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:258: }
On 2017/03/31 07:17:14, the sun wrote:
> I don't see a test where you have both send+receive modules, to test
> PacketRouter::SendFeedback().

I've changed rtp_2 to be registered as a receive module here.
Is that enough?

[stefan-webrtc, 2017-03-31 10:24:04]

lgtm

[the sun, 2017-03-31 10:38:48]

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:124: rtc::CritScope cs(&modules_crit_);
On 2017/03/31 08:30:54, nisse-webrtc wrote:
> On 2017/03/29 13:58:36, nisse-webrtc wrote:
> > On 2017/03/28 12:21:52, the sun wrote:
> > > From which thread is this driven, vs
TimeToSendPacket()/TimeToSendPadding()?
> > > Will there be contention on this lock?
> > 
> > Not entirely sure, but I think TimeToSendPacket is |Call::pacer_thread_|,
and
> > SendFeedback is |Call::module_process_thread_|.
> 
> After a second look, I think SendFeedback is actually called by the pacer
> thread. In which case the lock shouldn't be necessary. I'll test that out in a
> separate cl.

Nice!

https://codereview.webrtc.org/2774623006/diff/180001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2774623006/diff/180001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:61:
rtp_receive_modules_.erase(std::remove(rtp_receive_modules_.begin(),
This doesn't look right. Did you mean std::find? std::remove will already have
removed all matching elements in the vector, and IIUC we only expect one to be
there. Why then erase from that point to the end of the vector?

[nisse-webrtc, 2017-03-31 10:51:53]

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2774623006/diff/120001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:124: rtc::CritScope cs(&modules_crit_);
On 2017/03/31 10:38:48, the sun wrote:
> On 2017/03/31 08:30:54, nisse-webrtc wrote:
> > On 2017/03/29 13:58:36, nisse-webrtc wrote:
> > > On 2017/03/28 12:21:52, the sun wrote:
> > > > From which thread is this driven, vs
> TimeToSendPacket()/TimeToSendPadding()?
> > > > Will there be contention on this lock?
> > > 
> > > Not entirely sure, but I think TimeToSendPacket is |Call::pacer_thread_|,
> and
> > > SendFeedback is |Call::module_process_thread_|.
> > 
> > After a second look, I think SendFeedback is actually called by the pacer
> > thread. In which case the lock shouldn't be necessary. I'll test that out in
a
> > separate cl.
> 
> Nice!

Unfortunately, the lock is still needed, for synchronization with the methods
modifying the lists. The other cl is https://codereview.webrtc.org/2787283003/,
it just adds a thread check, and that change is incorporated in this cl too.

But at least there shouldn't be any contention once all streams are up and
running.

https://codereview.webrtc.org/2774623006/diff/180001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2774623006/diff/180001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:61:
rtp_receive_modules_.erase(std::remove(rtp_receive_modules_.begin(),
On 2017/03/31 10:38:48, the sun wrote:
> This doesn't look right. Did you mean std::find? std::remove will already have
> removed all matching elements in the vector, and IIUC we only expect one to be
> there. Why then erase from that point to the end of the vector?

My understanding is that std::remove moves elements around, but doesn't change
the size of the container. See https://en.wikipedia.org/wiki/Erase–remove_idiom.

Do you know any better and more concise way to remove a single element by value
from a vector?

[the sun, 2017-03-31 10:59:57]

lgtm

https://codereview.webrtc.org/2774623006/diff/180001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2774623006/diff/180001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:61:
rtp_receive_modules_.erase(std::remove(rtp_receive_modules_.begin(),
On 2017/03/31 10:51:53, nisse-webrtc wrote:
> On 2017/03/31 10:38:48, the sun wrote:
> > This doesn't look right. Did you mean std::find? std::remove will already
have
> > removed all matching elements in the vector, and IIUC we only expect one to
be
> > there. Why then erase from that point to the end of the vector?
> 
> My understanding is that std::remove moves elements around, but doesn't change
> the size of the container. See
https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom.

Ah, now I remember! Thanks. (I don't think I'll ever *really* make friends with
STL...)

> Do you know any better and more concise way to remove a single element by
value
> from a vector?

no, but doesn't std::find() seem more to-the-point here? ...and it would avoid
the one extra copy.

[nisse-webrtc, 2017-03-31 11:14:08]

https://codereview.webrtc.org/2774623006/diff/180001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2774623006/diff/180001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:61:
rtp_receive_modules_.erase(std::remove(rtp_receive_modules_.begin(),
On 2017/03/31 10:59:57, the sun wrote:
> no, but doesn't std::find() seem more to-the-point here? ...and it would avoid
> the one extra copy.

Since we already have the find call above (in the DCHECK), I guess it makes
sense to use its return value.

Copying will be the same, but at least we should examine the elements preceding
the element to be deleted only once.

[nisse-webrtc, 2017-03-31 11:15:28]

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

[commit-bot: I haz the power, 2017-03-31 11:15:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-31 11:57:26]

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

[commit-bot: I haz the power, 2017-03-31 11:57:27]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-03-31 12:41:34]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-03-31 12:41:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2774623006/#ps200001 (title:
"Eliminate std::remove.")

[commit-bot: I haz the power, 2017-03-31 12:41:42]

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-31 12:44:53]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490964094670190,
"parent_rev": "ec6fbd277623cc60007d6a57d91f1e6b78ad6b04", "commit_rev":
"fdbfdc9786de5ec50ff750e5a61c3d39524e24bb"}

[commit-bot: I haz the power, 2017-03-31 12:44:57]

Description was changed from

==========
Let PacketRouter separate send and receive modules.

This is in preparation for merging the ViERemb logic in packet_router,
to send REMB feedback as sender reports if possible, otherwise as
receiver reports.

BUG=webrtc:7135
==========

to

==========
Let PacketRouter separate send and receive modules.

This is in preparation for merging the ViERemb logic in packet_router,
to send REMB feedback as sender reports if possible, otherwise as
receiver reports.

BUG=webrtc:7135

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

[commit-bot: I haz the power, 2017-03-31 12:44:59]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/external/webrtc/+/fdbfdc9786de5ec50ff750e5a...