Remove BundleFilter filtering of RTCP.

Remove BundleFilter filtering of RTCP.

BundleFilter may not know the remote SSRC for all incoming RTCP packets,
so there's no point in filtering them.

BUG=webrtc:4740
R=hta@webrtc.org, juberti@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/482b12e2c3fedfe94a7c3fd665cbe77b848f1b31
Cr-Commit-Position: refs/heads/master@{#10655}

[pbos-webrtc, 2015-11-12 13:41:39]

PTAL, this looks like it's been a hot topic again. Might as well get it up for
discussion (so we can consider rolling out proper random SSRCs as well).

[pbos-webrtc, 2015-11-12 13:41:44]

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

[commit-bot: I haz the power, 2015-11-12 13:41:47]

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

[commit-bot: I haz the power, 2015-11-12 13:50:42]

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

[commit-bot: I haz the power, 2015-11-12 13:50:43]

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

[pbos-webrtc, 2015-11-12 15:47:25]

Please review even with the test failures, if we agree I'll fix the test
failures. :)

[hta-webrtc, 2015-11-12 16:47:53]

It's shorter code. I like it.

https://codereview.webrtc.org/1437683005/diff/1/talk/session/media/bundlefilt...
File talk/session/media/bundlefilter.h (right):

https://codereview.webrtc.org/1437683005/diff/1/talk/session/media/bundlefilt...
talk/session/media/bundlefilter.h:49: // this is decided based on the sender
ssrc values.
You need to change this comment too.

https://codereview.webrtc.org/1437683005/diff/1/talk/session/media/bundlefilt...
talk/session/media/bundlefilter.h:56: bool DemuxPacket(const uint8_t* data,
size_t len);
Since that's not what it does any more, what about renaming this function to
"CheckIfPayloadTypeIsValid"?

[pthatcher1, 2015-11-12 20:49:22]

lgtm, assuming you address Harald's comments

[juberti, 2015-11-13 07:34:23]

juberti@google.com changed reviewers:
+ juberti@google.com

[juberti, 2015-11-13 07:34:23]

lgtm... the time for this has finally come.

https://codereview.webrtc.org/1437683005/diff/1/talk/session/media/bundlefilt...
File talk/session/media/bundlefilter.h (right):

https://codereview.webrtc.org/1437683005/diff/1/talk/session/media/bundlefilt...
talk/session/media/bundlefilter.h:56: bool DemuxPacket(const uint8_t* data,
size_t len);
On 2015/11/12 16:47:53, hta-webrtc wrote:
> Since that's not what it does any more, what about renaming this function to
> "CheckIfPayloadTypeIsValid"?

This name still seems fine to me. In the future this code may need to demux by
MID extension, so no point in changing the name to something less general.

[juberti, 2015-11-13 07:40:02]

Note that this code will cause us to start dual-decrypting RTCP packets in
bundle contexts, and feeding them downwards.

This is probably acceptable, but could lead to some unexpected behavior. 

One solution could be to build the SSRC table based on packets we have sent.
e.g. if we send RTP from SSRC X, allow RTCP to SSRC X.

[hta-webrtc, 2015-11-13 10:25:00]

On 2015/11/13 07:40:02, juberti wrote:
> Note that this code will cause us to start dual-decrypting RTCP packets in
> bundle contexts, and feeding them downwards.

I'm confused about this comment. Do you mean that we would decrypt them twice,
and that we currently don't?
 
> This is probably acceptable, but could lead to some unexpected behavior. 
> 
> One solution could be to build the SSRC table based on packets we have sent.
> e.g. if we send RTP from SSRC X, allow RTCP to SSRC X.

RTCPs don't have TO, though.
They have FROM (which was a design mistake, since it confuses recipient ID with
stream ID, and is a direct cause of the irritating situation this CL is about)
and ABOUT (which is embedded in each SR or RR in the body of the message).

It would make sense to have a single point in the code that takes apart incoming
SSRCs and dispatches the individual SR, RR and other packets (like our
transport-wide arrival time recording feedback message) and feeds it to the
entity that's interested in them (stats counters for the RR and SR, congestion
control for the arrival-time stuff).

But that's not in scope for this CL, so we should probably stop discussing that
on this thread.

[pbos-webrtc, 2015-11-13 10:28:03]

On 2015/11/13 10:25:00, hta-webrtc wrote:
> On 2015/11/13 07:40:02, juberti wrote:
> > Note that this code will cause us to start dual-decrypting RTCP packets in
> > bundle contexts, and feeding them downwards.
> 
> I'm confused about this comment. Do you mean that we would decrypt them twice,
> and that we currently don't?
>  
> > This is probably acceptable, but could lead to some unexpected behavior. 
> > 
> > One solution could be to build the SSRC table based on packets we have sent.
> > e.g. if we send RTP from SSRC X, allow RTCP to SSRC X.
> 
> RTCPs don't have TO, though.
> They have FROM (which was a design mistake, since it confuses recipient ID
with
> stream ID, and is a direct cause of the irritating situation this CL is about)
> and ABOUT (which is embedded in each SR or RR in the body of the message).
> 
> It would make sense to have a single point in the code that takes apart
incoming
> SSRCs and dispatches the individual SR, RR and other packets (like our
> transport-wide arrival time recording feedback message) and feeds it to the
> entity that's interested in them (stats counters for the RR and SR, congestion
> control for the arrival-time stuff).
> 
> But that's not in scope for this CL, so we should probably stop discussing
that
> on this thread.

With webrtc::Call we'll have one point of entry, so as soon as VoiceEngine has
been converted over to webrtc::Call (as AudioSendStream, AudioReceiveStream) we
can probably just merge webrtcvideoengine2.cc and webrtcvoiceengine.cc into
webrtcmediaengine.cc and stop having these distinctions. This would avoid the
double-decrypt as well. :)

[pbos-webrtc, 2015-11-13 11:06:47]

test fixes

[pbos-webrtc, 2015-11-13 11:09:41]

update comment

[pbos-webrtc, 2015-11-13 11:10:16]

PTAL

https://codereview.webrtc.org/1437683005/diff/1/talk/session/media/bundlefilt...
File talk/session/media/bundlefilter.h (right):

https://codereview.webrtc.org/1437683005/diff/1/talk/session/media/bundlefilt...
talk/session/media/bundlefilter.h:49: // this is decided based on the sender
ssrc values.
On 2015/11/12 16:47:53, hta-webrtc wrote:
> You need to change this comment too.

Done.

https://codereview.webrtc.org/1437683005/diff/1/talk/session/media/bundlefilt...
talk/session/media/bundlefilter.h:56: bool DemuxPacket(const uint8_t* data,
size_t len);
On 2015/11/13 07:34:23, juberti wrote:
> On 2015/11/12 16:47:53, hta-webrtc wrote:
> > Since that's not what it does any more, what about renaming this function to
> > "CheckIfPayloadTypeIsValid"?
> 
> This name still seems fine to me. In the future this code may need to demux by
> MID extension, so no point in changing the name to something less general.
> 

Going with not changing it, but I changed the comment to mention RTP packets.

[pbos-webrtc, 2015-11-13 11:13:27]

Description was changed from

==========
Remove BundleFilter filtering of RTCP.

BundleFilter may not know the remote SSRC for all incoming RTCP packets,
so there's no point in filtering them.

BUG=webrtc:4740
R=hta@webrtc.org, pthatcher@webrtc.org
==========

to

==========
Remove BundleFilter filtering of RTCP.

BundleFilter may not know the remote SSRC for all incoming RTCP packets,
so there's no point in filtering them.

BUG=webrtc:4740
R=hta@webrtc.org, juberti@webrtc.org, pthatcher@webrtc.org
==========

[pbos-webrtc, 2015-11-13 11:13:28]

Description was changed from

==========
Remove BundleFilter filtering of RTCP.

BundleFilter may not know the remote SSRC for all incoming RTCP packets,
so there's no point in filtering them.

BUG=webrtc:4740
R=hta@webrtc.org, pthatcher@webrtc.org
==========

to

==========
Remove BundleFilter filtering of RTCP.

BundleFilter may not know the remote SSRC for all incoming RTCP packets,
so there's no point in filtering them.

BUG=webrtc:4740
R=hta@webrtc.org, juberti@webrtc.org, pthatcher@webrtc.org
==========

[pbos-webrtc, 2015-11-13 11:14:43]

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

[commit-bot: I haz the power, 2015-11-13 11:14:53]

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

[commit-bot: I haz the power, 2015-11-13 12:28:13]

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

[commit-bot: I haz the power, 2015-11-13 12:28:13]

Dry run: This issue passed the CQ dry run.

[juberti, 2015-11-16 06:32:17]

On 2015/11/13 10:28:03, pbos-webrtc wrote:
> On 2015/11/13 10:25:00, hta-webrtc wrote:
> > On 2015/11/13 07:40:02, juberti wrote:
> > > Note that this code will cause us to start dual-decrypting RTCP packets in
> > > bundle contexts, and feeding them downwards.
> > 
> > I'm confused about this comment. Do you mean that we would decrypt them
twice,
> > and that we currently don't?
> >  
> > > This is probably acceptable, but could lead to some unexpected behavior. 
> > > 
> > > One solution could be to build the SSRC table based on packets we have
sent.
> > > e.g. if we send RTP from SSRC X, allow RTCP to SSRC X.
> > 
> > RTCPs don't have TO, though.
> > They have FROM (which was a design mistake, since it confuses recipient ID
> with
> > stream ID, and is a direct cause of the irritating situation this CL is
about)
> > and ABOUT (which is embedded in each SR or RR in the body of the message).
> > 
> > It would make sense to have a single point in the code that takes apart
> incoming
> > SSRCs and dispatches the individual SR, RR and other packets (like our
> > transport-wide arrival time recording feedback message) and feeds it to the
> > entity that's interested in them (stats counters for the RR and SR,
congestion
> > control for the arrival-time stuff).
> > 
> > But that's not in scope for this CL, so we should probably stop discussing
> that
> > on this thread.
> 
> With webrtc::Call we'll have one point of entry, so as soon as VoiceEngine has
> been converted over to webrtc::Call (as AudioSendStream, AudioReceiveStream)
we
> can probably just merge webrtcvideoengine2.cc and webrtcvoiceengine.cc into
> webrtcmediaengine.cc and stop having these distinctions. This would avoid the
> double-decrypt as well. :)

yes, I think that would be the most reasonable long-term solution here as well.

[pbos-webrtc, 2015-11-16 16:43:06]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2015-11-16 16:43:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, juberti@google.com
Link to the patchset: https://codereview.webrtc.org/1437683005/#ps40001 (title:
"update comment")

[commit-bot: I haz the power, 2015-11-16 16:43:12]

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

[commit-bot: I haz the power, 2015-11-16 18:20:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-16 18:20:13]

Patchset 3 (id:??) landed as
https://crrev.com/482b12e2c3fedfe94a7c3fd665cbe77b848f1b31
Cr-Commit-Position: refs/heads/master@{#10655}