Move RTP/RTCP demuxing logic from BaseChannel to RtpTransport.

Move RTP/RTCP demuxing logic from BaseChannel to RtpTransport.

BUG=webrtc:7013
Review-Url: https://codereview.webrtc.org/2890263003
Cr-Commit-Position: refs/heads/master@{#18391}
Committed: https://chromium.googlesource.com/external/webrtc/+/3dcf0e93fa508c316f3c79fa171ee50b144b3195

[Zach Stein, 2017-05-24 21:49:16]

zstein@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[Zach Stein, 2017-05-24 21:49:17]

Subsumes https://codereview.chromium.org/2889453002/ - Move bundle filter from
BaseChannel to RtpTransport.

RtpTransport::PacketIsRtcp doesn't check that rtcp mux is enabled right now (see
code). Not sure if that will be a problem or not.

The test cases in rtcpmuxfilter_unittests should be changed to call IsActive
instead of DemuxRtcp (they are removed in this iteration - I will look at
putting them back now).

[Taylor Brandstetter, 2017-05-25 16:14:25]

I think BaseChannel should keep track of the "first packet received" stuff
itself, since it can be easily determined using SignalPacketReceived, and this
would avoid adding an extra method and signal to RtpTransport's interface.

Otherwise this looks good; just needs some tests.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/media/base/rtputils.h
File webrtc/media/base/rtputils.h (right):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/media/base/rtputil...
webrtc/media/base/rtputils.h:63: const char* PacketType(bool rtcp);
Can you add a comment above these declarations? And if we don't have one
somewhere else, can you add a TODO to use an enum to differentiate RTP/RTCP
instead of a bool?

Also, since PacketType has a larger scope now, a more descriptive name would be
good.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:178: // callback interface later so that the demuxer can
select which chanenl
nit: "chanenl"

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtcpmuxfilter_u...
File webrtc/pc/rtcpmuxfilter_unittest.cc (left):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtcpmuxfilter_u...
webrtc/pc/rtcpmuxfilter_unittest.cc:79: }
We should replace these with equivalent tests in rtptransport_unittest.cc

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:183: SignalFirstPacketReceived();
nit: Since ultimately "SignalFirstPacketReceived" turns into an RtpReceiver
signal (meaning it ultimately needs to be per-receiver instead of
per-transport), I think it would make more sense to keep this logic confined to
BaseChannel.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:192: received_media_ = true;
Same sort of situation here. VoiceChannel already has the information it needs
to determine "received media", so storing this flag in RtpTransport doesn't seem
to add value. And later, when it will be possible to have multiple VoiceChannels
(that need independent "received media" flags) using the same RtpTransport, this
would become more complicated.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:198: bool IsRtcp(const char* data, int len) {
If we're moving things to rtputils.h, this would be a good candidate.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:225: // rely on IsRtcp (and not bother checking
rtcp_mux_enabled)?
Let's just do this. The only way we could *not offer RTCP mux is if the
application manually removes "a=rtcp-mux" from the offer. So receiving a
RTCP-muxed packet when RTCP mux wasn't offered is a corner case of a corner
case, which will eventually not even be possible, and thus isn't worth doing
anything about.

[Zach Stein, 2017-05-30 21:50:55]

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/media/base/rtputils.h
File webrtc/media/base/rtputils.h (right):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/media/base/rtputil...
webrtc/media/base/rtputils.h:63: const char* PacketType(bool rtcp);
On 2017/05/25 16:14:25, Taylor Brandstetter wrote:
> Can you add a comment above these declarations? And if we don't have one
> somewhere else, can you add a TODO to use an enum to differentiate RTP/RTCP
> instead of a bool?
> 
> Also, since PacketType has a larger scope now, a more descriptive name would
be
> good.

Done.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:178: // callback interface later so that the demuxer can
select which chanenl
On 2017/05/25 16:14:25, Taylor Brandstetter wrote:
> nit: "chanenl"

Dnoe.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtcpmuxfilter_u...
File webrtc/pc/rtcpmuxfilter_unittest.cc (left):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtcpmuxfilter_u...
webrtc/pc/rtcpmuxfilter_unittest.cc:79: }
On 2017/05/25 16:14:25, Taylor Brandstetter wrote:
> We should replace these with equivalent tests in rtptransport_unittest.cc

I don't think these tests are needed anymore. There are tests below for the
IsActive methods and we don't use rtcp_mux_enabled_ to decide whether to demux
in RtpTransport anymore. Do you think the new Signal...FromBundled tests are
sufficient?

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:183: SignalFirstPacketReceived();
On 2017/05/25 16:14:25, Taylor Brandstetter wrote:
> nit: Since ultimately "SignalFirstPacketReceived" turns into an RtpReceiver
> signal (meaning it ultimately needs to be per-receiver instead of
> per-transport), I think it would make more sense to keep this logic confined
to
> BaseChannel.

Done.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:192: received_media_ = true;
On 2017/05/25 16:14:25, Taylor Brandstetter wrote:
> Same sort of situation here. VoiceChannel already has the information it needs
> to determine "received media", so storing this flag in RtpTransport doesn't
seem
> to add value. And later, when it will be possible to have multiple
VoiceChannels
> (that need independent "received media" flags) using the same RtpTransport,
this
> would become more complicated.

Done.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:198: bool IsRtcp(const char* data, int len) {
On 2017/05/25 16:14:25, Taylor Brandstetter wrote:
> If we're moving things to rtputils.h, this would be a good candidate.

I was only moving methods that are called from multiple files to rtputils. I was
thinking they might end up being only in rtptransport.cc long term, but don't
feel strongly.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:225: // rely on IsRtcp (and not bother checking
rtcp_mux_enabled)?
On 2017/05/25 16:14:25, Taylor Brandstetter wrote:
> Let's just do this. The only way we could *not offer RTCP mux is if the
> application manually removes "a=rtcp-mux" from the offer. So receiving a
> RTCP-muxed packet when RTCP mux wasn't offered is a corner case of a corner
> case, which will eventually not even be possible, and thus isn't worth doing
> anything about.

Done.

Am I understanding correctly that the existing implementation could interpret an
rtp packet incorrectly as an rtcp packet if mux is offered but not supported by
the remote client and the rtp packet is received before the answer?

[Zach Stein, 2017-05-30 21:51:51]

Description was changed from

==========
WIP move demuxing logic from BaseChannel to RtpTransport.

BUG=webrtc:7013
==========

to

==========
Move RTP/RTCP demuxing logic from BaseChannel to RtpTransport.

BUG=webrtc:7013
==========

[Taylor Brandstetter, 2017-05-31 08:25:03]

lgtm

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtcpmuxfilter_u...
File webrtc/pc/rtcpmuxfilter_unittest.cc (left):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtcpmuxfilter_u...
webrtc/pc/rtcpmuxfilter_unittest.cc:79: }
On 2017/05/30 21:50:55, Zach Stein wrote:
> On 2017/05/25 16:14:25, Taylor Brandstetter wrote:
> > We should replace these with equivalent tests in rtptransport_unittest.cc
> 
> I don't think these tests are needed anymore. There are tests below for the
> IsActive methods and we don't use rtcp_mux_enabled_ to decide whether to demux
> in RtpTransport anymore. Do you think the new Signal...FromBundled tests are
> sufficient?

The new "Signal..." tests are just what I had in mind.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:225: // rely on IsRtcp (and not bother checking
rtcp_mux_enabled)?
On 2017/05/30 21:50:55, Zach Stein wrote:
> On 2017/05/25 16:14:25, Taylor Brandstetter wrote:
> > Let's just do this. The only way we could *not offer RTCP mux is if the
> > application manually removes "a=rtcp-mux" from the offer. So receiving a
> > RTCP-muxed packet when RTCP mux wasn't offered is a corner case of a corner
> > case, which will eventually not even be possible, and thus isn't worth doing
> > anything about.
> 
> Done.
> 
> Am I understanding correctly that the existing implementation could interpret
an
> rtp packet incorrectly as an rtcp packet if mux is offered but not supported
by
> the remote client and the rtp packet is received before the answer?

Even then, the remote client should only use payload types we offered... which
won't be in the range that would cause the packet to be interpreted as RTCP. I
don't think there's any situation where this is actually an issue.

https://codereview.webrtc.org/2890263003/diff/80001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2890263003/diff/80001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:191: // This mutates packet if it is protected.
nit: We typically enclose variable names in '|'s in comments, so it's more clear
they're variable names.

https://codereview.webrtc.org/2890263003/diff/80001/webrtc/pc/rtptransport.h
File webrtc/pc/rtptransport.h (right):

https://codereview.webrtc.org/2890263003/diff/80001/webrtc/pc/rtptransport.h#...
webrtc/pc/rtptransport.h:74: // RtcpPacketReceived.
Could mention that the first bool argument of the signal is "true for RTCP,
false for RTP".

[Zach Stein, 2017-06-01 03:45:58]

Thanks.

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2890263003/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:225: // rely on IsRtcp (and not bother checking
rtcp_mux_enabled)?
On 2017/05/31 08:25:03, Taylor Brandstetter wrote:
> On 2017/05/30 21:50:55, Zach Stein wrote:
> > On 2017/05/25 16:14:25, Taylor Brandstetter wrote:
> > > Let's just do this. The only way we could *not offer RTCP mux is if the
> > > application manually removes "a=rtcp-mux" from the offer. So receiving a
> > > RTCP-muxed packet when RTCP mux wasn't offered is a corner case of a
corner
> > > case, which will eventually not even be possible, and thus isn't worth
doing
> > > anything about.
> > 
> > Done.
> > 
> > Am I understanding correctly that the existing implementation could
interpret
> an
> > rtp packet incorrectly as an rtcp packet if mux is offered but not supported
> by
> > the remote client and the rtp packet is received before the answer?
> 
> Even then, the remote client should only use payload types we offered... which
> won't be in the range that would cause the packet to be interpreted as RTCP. I
> don't think there's any situation where this is actually an issue.

Acknowledged.

https://codereview.webrtc.org/2890263003/diff/80001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2890263003/diff/80001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:191: // This mutates packet if it is protected.
On 2017/05/31 08:25:03, Taylor Brandstetter wrote:
> nit: We typically enclose variable names in '|'s in comments, so it's more
clear
> they're variable names.

Done.

https://codereview.webrtc.org/2890263003/diff/80001/webrtc/pc/rtptransport.h
File webrtc/pc/rtptransport.h (right):

https://codereview.webrtc.org/2890263003/diff/80001/webrtc/pc/rtptransport.h#...
webrtc/pc/rtptransport.h:74: // RtcpPacketReceived.
On 2017/05/31 08:25:03, Taylor Brandstetter wrote:
> Could mention that the first bool argument of the signal is "true for RTCP,
> false for RTP".

Done.

[Zach Stein, 2017-06-01 03:46:07]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-06-01 03:46:07]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2890263003/#ps100001 (title:
"Clean up some comments as suggested by Taylor.")

[commit-bot: I haz the power, 2017-06-01 03:46:11]

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-06-01 03:51:00]

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

[commit-bot: I haz the power, 2017-06-01 03:51:01]

Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/19382)
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/26558)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/25957)

[Zach Stein, 2017-06-01 05:29:36]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-06-01 05:29:37]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2890263003/#ps120001 (title:
"Fix warning C4309 (truncation of constant value).")

[commit-bot: I haz the power, 2017-06-01 05:29:47]

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-06-01 05:36:51]

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

[commit-bot: I haz the power, 2017-06-01 05:36:52]

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

[Zach Stein, 2017-06-01 05:39:04]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-06-01 05:39:04]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2890263003/#ps140001 (title:
"Really fix warning C4309.")

[commit-bot: I haz the power, 2017-06-01 05:39:15]

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-06-01 05:44:58]

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

[commit-bot: I haz the power, 2017-06-01 05:44:59]

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

[Zach Stein, 2017-06-01 17:56:45]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-06-01 17:56:46]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2890263003/#ps180001 (title:
"Remove fixed TODO.")

[commit-bot: I haz the power, 2017-06-01 17:56:54]

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-06-01 18:28:37]

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

[commit-bot: I haz the power, 2017-06-01 18:28:38]

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

[pthatcher1, 2017-06-01 18:49:16]

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

[pthatcher1, 2017-06-01 18:49:17]

https://codereview.webrtc.org/2890263003/diff/180001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2890263003/diff/180001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:214: }
I think you need rtp_transport_->SetRtpPacketTransport(null) here.

[Zach Stein, 2017-06-01 19:56:40]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-06-01 19:56:40]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2890263003/#ps200001 (title:
"Disconnect transport channels in method called from Deinit to prevent races
during object destructi…")

[commit-bot: I haz the power, 2017-06-01 19:56:50]

CQ is trying da patch.

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

[Zach Stein, 2017-06-01 19:56:54]

https://codereview.webrtc.org/2890263003/diff/180001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2890263003/diff/180001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:214: }
On 2017/06/01 18:49:17, pthatcher1 wrote:
> I think you need rtp_transport_->SetRtpPacketTransport(null) here.

Done. Thanks for helping me track this down.
This should fix the race tsan caught.

[commit-bot: I haz the power, 2017-06-01 20:22:43]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1496347000100080,
"parent_rev": "7d9a55b92db2f41b597a084e00853e017d94000f", "commit_rev":
"3dcf0e93fa508c316f3c79fa171ee50b144b3195"}

[commit-bot: I haz the power, 2017-06-01 20:22:46]

Description was changed from

==========
Move RTP/RTCP demuxing logic from BaseChannel to RtpTransport.

BUG=webrtc:7013
==========

to

==========
Move RTP/RTCP demuxing logic from BaseChannel to RtpTransport.

BUG=webrtc:7013

Review-Url: https://codereview.webrtc.org/2890263003
Cr-Commit-Position: refs/heads/master@{#18391}
Committed:
https://chromium.googlesource.com/external/webrtc/+/3dcf0e93fa508c316f3c79fa1...
==========

[commit-bot: I haz the power, 2017-06-01 20:22:48]

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